Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: foreign proposal transaction timeout #802

Merged
merged 4 commits into from
Feb 5, 2024

Conversation

Cifko
Copy link
Contributor

@Cifko Cifko commented Dec 1, 2023

Description

Transaction (from foreign proposals) timeout.
For every Foreign proposal we store new column with all the transactions id. Once we receive local block where the transaction should be resolved and it's not (it's not finalized, or the current state is not at least LocalPrepared) we update the local decision to abort.

Motivation and Context

How Has This Been Tested?

N/A

What process can a PR reviewer use to test or verify this change?

Breaking Changes

  • None
  • Requires data directory to be deleted
  • Other - Please specify

Copy link

github-actions bot commented Dec 1, 2023

Test Results (CI)

447 tests  ±0   447 ✅ ±0   2h 9m 6s ⏱️ + 5m 5s
 57 suites ±0     0 💤 ±0 
  2 files   ±0     0 ❌ ±0 

Results for commit 49217a3. ± Comparison against base commit 76d052c.

♻️ This comment has been updated with latest results.

@Cifko Cifko changed the title feat: timeout feat: foreign proposal transaction timeout Dec 1, 2023
@Cifko Cifko force-pushed the timeout branch 3 times, most recently from f0fc700 to 7612699 Compare January 12, 2024 09:56
@Cifko Cifko force-pushed the timeout branch 3 times, most recently from 2dce482 to 1e50f4e Compare January 25, 2024 09:26
stringhandler
stringhandler previously approved these changes Jan 26, 2024
Copy link
Contributor

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK, seems correct

Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
I don't think we should be using 'mined` when talking about L2 blocks.

@@ -74,6 +74,7 @@ message ForeignProposal {
bytes block_id = 2;
ForeignProposalState state = 3;
uint64 mined_at = 4;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest proposed_height. This has already changed in development but I missed this in the proto file

Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

Tested but only with a single shard

@sdbondi sdbondi added this pull request to the merge queue Feb 5, 2024
Merged via the queue into tari-project:development with commit 9a9bd6e Feb 5, 2024
11 checks passed
sdbondi added a commit to sdbondi/tari-dan that referenced this pull request Feb 5, 2024
* development:
  feat: foreign proposal transaction timeout (tari-project#802)
  fix: use hash domains from tari_hash_domains (tari-project#925)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants