fuzz: Avoid timeout in utxo_total_supply #27780
Merged
Conversation
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
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
May 31, 2023
fafb4da fuzz: Avoid timeout in utxo_total_supply (MarcoFalke) Pull request description: Looks like for high block counts it may be better to mock the chain, otherwise a high limit will lead to fuzz input bloat and timeouts, see bitcoin#17860 (comment). It can be checked that the fuzz target can still find the CVE, see bitcoin#17860 (review) with a diff of: ```diff diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp index f949655909..6f4cfb5f51 100644 --- a/src/consensus/tx_check.cpp +++ b/src/consensus/tx_check.cpp @@ -39,8 +39,6 @@ bool CheckTransaction(const CTransaction& tx, TxValidationState& state) // the underlying coins database. std::set<COutPoint> vInOutPoints; for (const auto& txin : tx.vin) { - if (!vInOutPoints.insert(txin.prevout).second) - return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-inputs-duplicate"); } if (tx.IsCoinBase()) ``` Also, fix a nit, see bitcoin#17860 (comment) ACKs for top commit: dergoegge: ACK fafb4da Tree-SHA512: a28fe9cd6ebb4c9bed5a5b35be76c1c436a87586c8fc3b3c4c8559a4a77ac08098324370da421d794c99579882c0872b6b29415de47ade6a05a08504a3d494c4
fanquake
added a commit
that referenced
this pull request
Jun 7, 2023
71200ac [fuzz] Only check duplicate coinbase script when block was valid (dergoegge) Pull request description: Partially revert #27780, because moving the duplicate coinbase check out of the `was_valid` branch leads to non-bug crashes in the fuzz target. For context and further explanation see: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=59516 ACKs for top commit: MarcoFalke: nice lgtm ACK 71200ac Tree-SHA512: 8c38e5ff9de6331016b9a0c5e435d007d46186151b04c09085f617bb31627a28ad56678066fe152372a3ad8656f026439e3e2f9ee61d7ef588072aef8124eaa3
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jun 7, 2023
71200ac [fuzz] Only check duplicate coinbase script when block was valid (dergoegge) Pull request description: Partially revert bitcoin#27780, because moving the duplicate coinbase check out of the `was_valid` branch leads to non-bug crashes in the fuzz target. For context and further explanation see: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=59516 ACKs for top commit: MarcoFalke: nice lgtm ACK 71200ac Tree-SHA512: 8c38e5ff9de6331016b9a0c5e435d007d46186151b04c09085f617bb31627a28ad56678066fe152372a3ad8656f026439e3e2f9ee61d7ef588072aef8124eaa3
fanquake
added a commit
to bitcoin-core/gui
that referenced
this pull request
Nov 7, 2023
…ake 2) fa7ba92 fuzz: Avoid utxo_total_supply timeout (MarcoFalke) Pull request description: Looks like this still may take a long time to run large fuzz inputs. Thus, reduce it further, but still allow it to catch the regression, if re-introduced: ```diff diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp index f949655909..4bdd15c5ee 100644 --- a/src/consensus/tx_check.cpp +++ b/src/consensus/tx_check.cpp @@ -40,7 +40,7 @@ bool CheckTransaction(const CTransaction& tx, TxValidationState& state) std::set<COutPoint> vInOutPoints; for (const auto& txin : tx.vin) { if (!vInOutPoints.insert(txin.prevout).second) - return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-inputs-duplicate"); + {}//return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-inputs-duplicate"); } if (tx.IsCoinBase()) ``` This is the second take, see bitcoin/bitcoin#27780. If in the future it still times out, I think the fuzz test can just be removed. Example input: ``` JREROy5pcnAgQyw7IC4ODg4ODg4ODg4O0dEODg4ODg4ZDg4ODg4ODg4ODg7RDg4ODg4ODg4O0dEODg4ODg4ODg4ODg7R0Q4ODg4ODg4ODtHRDg4ODtHR0dEODg4O0dEODg7R0Q4ODg4ODg4ODtHRDg4ODg4ODg4ODg4O0dEODg4ODg4ODg7R0Q4ODg7R0Q4O0dEODg4ODg4ODg4ODg7R0Q4ODg4ODtHRDg4ODtHR ACKs for top commit: dergoegge: ACK fa7ba92 brunoerg: utACK fa7ba92 Tree-SHA512: 154a4895834babede6ce7b775562a7026637af1097e53e55676e92f6cf966ae0c092300ebf7e51a397eebd11f7b41d020586663e781f70d084efda1c0fe851b4
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Looks like for high block counts it may be better to mock the chain, otherwise a high limit will lead to fuzz input bloat and timeouts, see #17860 (comment).
It can be checked that the fuzz target can still find the CVE, see #17860 (review) with a diff of:
Also, fix a nit, see #17860 (comment)