-
Notifications
You must be signed in to change notification settings - Fork 110
fix: mempool occasional panics when store is ahead by a block #1984
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
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
84867be
Fix: mempool desync panic
Mirko-von-Leipzig eb3489b
Revert "Fix: mempool desync panic"
Mirko-von-Leipzig 8f63c4c
Consider pending block as chain tip in mempool
Mirko-von-Leipzig d3d2ece
changelog
Mirko-von-Leipzig 0109f22
Review comments
Mirko-von-Leipzig 5dceb94
Ensure add_transaction and add_batch return the committed tip
Mirko-von-Leipzig 962239a
Merge branch 'main' into mirko/fix-mempool-panic
bobbinth 67e763e
Merge branch 'main' into mirko/fix-mempool-panic
Mirko-von-Leipzig File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we may want to rename this. Reading the code, some places still use
self.chain_tipand some others have moved toself.chain_tip(), so the difference is not immediately obvious to the reader. Or maybe the field itself could be renamed (committed_chain_tip?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. I think renaming the field is a great idea, thank you!
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@igamigo on the point of the RPC now returning the pending block height.. are you saying that's okay? This was an unintended side effect for me which I am going to revert.
But it sounds like you're in favor of it, or okay with it? My main concern would be that its possible this block does not become committed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I went back and forth, but I think we want to remove this as well. As I said, that return value is supposed to be a hint for clients to know after which block their transaction would be committed (and so, when output notes would be created in the chain). Since that block number may not be committed, it may not be always accurate (ie we may return block 5 but it may turn out that the transaction we are submitting is committed in block 5).