-
Notifications
You must be signed in to change notification settings - Fork 35.4k
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
AssumeUTXO follow-ups #28562
Merged
Merged
AssumeUTXO follow-ups #28562
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
0a39b8c
validation: remove unused mempool param in DetectSnapshotChainstate
fjahr a47fbe7
doc: Add and edit some comments around assumeutxo
fjahr 4e915e9
test: Improvements of feature_assumeutxo
fjahr 2c9354f
doc: Add snapshot chainstate removal warning to reindexing documentation
fjahr 73700fb
validation, test: Improve and document nChainTx check for testability
fjahr 82e48d2
blockstorage: Let FlushChainstateBlockFile return true in case of mis…
fjahr a482f86
chain: Rename HaveTxsDownloaded to HaveNumChainTxs
fjahr 1ff1c34
test: Rename wait_until_helper to wait_until_helper_internal
fjahr 710e5db
doc: Drop references to assumevalid in assumeutxo docs
fjahr 5d227a6
rpc: Use Ensure(Any)Chainman in assumeutxo related RPCs
fjahr File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains 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 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 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
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.
Which testing do you mean?
-chain=main -checkblockindex
on mainnet?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.
The tests that are changed in the same commit.
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 removed the "testing" check and ran the main binary (not the tests) and it crashed here.
Generally I don't think a good place to put test-only code is the main production code. Especially when it comes to consensus critical code. This makes it impossible to properly test and review the code in a production environment outside of unit tests.
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.
Hm, can you say what exactly you removed and what you can to see the crash? I couldn't reproduce that so far.
I guess we should improve our test setup to include realistic
nTx
andnChainTx
values so we can remove those checks again.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 used this diff IIRC, which also fails the functional tests:
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.
Another report, which could replicate the crash, see ##28791
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.
Turned into an issue: #29261