Miniminer linearization followups #4
Closed
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
This reflects what the script does (docker run ...).
The same is done by the 06 script.
Force remove any containers, pontentially leaving dangling processes, which should be fine.
This limits the scope of the CI_CONTAINER_ID symbol. Can be reviewed with --color-moved=dimmed-zebra
InferPubkey can return a nullptr, so check it's result before continuing with creating the inferred descriptor.
Descriptors disallows hybrid pubkeys. Anything with hybrid pubkeys should becomes a raw() descriptor that shows up in the watchonly wallet.
… check disk space periodically ed52e71 Periodically check disk space to avoid corruption (Aurèle Oulès) 7fe537f Implement CCoinsViewErrorCatcher::HaveCoin (Aurèle Oulès) Pull request description: Attempt to fix bitcoin#26112. As suggested by sipa in bitcoin#26112 (comment): > CCoinsViewErrorCatcher, the wrapper class used around CCoinsViewDB that's supposed to detect these problems and forcefully exit the application, has an override for GetCoins. But in CheckTxInputs, HaveInputs is first invoked, which on its turn calls HaveCoin. HaveCoin is implemented in CCoinsViewDB, but not in CCoinsViewErrorCatcher, and thus the disk read exception escapes. > A solution may be to just add an override for HaveCoin in CCoinsViewErrorCatcher. I implemented `CCoinsViewErrorCatcher::HaveCoin` and also added a periodic disk space check that shutdowns the node if there is not enough space left on disk, the minimum here is 50MB. For reviewers, it's possible to saturate disk space to test the PR by creating large files with `fallocate -l 50G test.bin` ACKs for top commit: achow101: ACK ed52e71 w0xlt: Code Review ACK bitcoin@ed52e71 sipa: utACK ed52e71 Tree-SHA512: 456aa7b996023df42b4fbb5158ee429d9abf7374b7b1ec129b21aea1188ad19be8da4ae8e0edd90b85b7a3042b8e44e17d3742e33808a4234d5ddbe9bcef1b78
Recently usage of undeclared functions became an error rather than a warning, in C2x. https://reviews.llvm.org/D122983?id=420290 This change has migrated into the build tools of Ubuntu 23.10 which now causes the USDT tests to fail to compile, see bitcoin#28600 Fix this by setting `-Wno-error=implicit-function-declaration` for the tracing programs.
I forgot to do this in 7d58152.
This is a simpler (less hardening) version of bitcoin#24123. Scoped to aarch64 to avoid unused command line option warnings when building on x86_64. Related to bitcoin#19075.
revert netutil chgs py3.8 compliant fixes based on PR review
Add Context Manager to manage wallet locking/unlocking with passphrase
…cted to Bitcoind nodes send getaddr msgs only to outbound nodes (and ignore those received by outgoing connections). The python p2p node should mirror this behavior by not sending a getaddr message when it is not the initiator of the connection.
8735e2c ci: use LLVM/Clang 17 in tidy job (fanquake) ce46b68 ci: use LLVM 17.0.2 in MSAN jobs (fanquake) Pull request description: Also update MSAN to use 17.0.2. Related to bitcoin#28465. ACKs for top commit: maflcko: lgtm ACK 8735e2c Tree-SHA512: 74452b95326cf065afe8332dc1b5b8e5ac12c8fe05c278a1cee017f87a7f7e0cdb8cac5e39d718c8ef587c8ee229bbaadd847df9f191313d41c5cdcab45e7c76
57131bf docs: fix typo (vuittont60) Pull request description: ACKs for top commit: maflcko: lgtm ACK 57131bf hebasto: ACK 57131bf, the `codespell` is powerless to catch this typo. jarolrod: ACK 57131bf Tree-SHA512: 816dfc5ff64531ea92acf35feca2286a71c75344df2524ff003a3d375e60100b8531e1678be0ed11863d03ab522d5733d8a0bf4b6f5f79c495a65246fe0b697f
…ys when inferring 74c7782 test: Unit test for inferring scripts with hybrid and uncompressed keys (Andrew Chow) f895f97 test: Scripts with hybrid pubkeys are migrated to watchonly (Andrew Chow) 37b9b73 descriptors: Move InferScript's pubkey validity checks to InferPubkey (Andrew Chow) b7485f1 descriptors: Check result of InferPubkey (Andrew Chow) Pull request description: `InferDescriptor` was not always checking that the pubkey it was placing into the descriptor was an allowed pubkey. For example, given a P2WPKH script that uses an uncompressed pubkey, it would produce a `wpkh()` with the uncompressed key. Additionally, the hybrid key check was only being done for `pk()` scripts, where it should've been done for all scripts. This PR moves the key checking into `InferPubkey`. If the key is not valid for the context, then `nullptr` is returned and the inferring will fall through to the defaults of either `raw()` or `addr()`. This also resolves an issue with migrating legacy wallets that contain hybrid pubkeys as such watchonly scripts will become `raw()` or `addr()` and go to the watchonly wallet. Note that a legacy wallet cannot sign for hybrid pubkeys. A test has been added for the migration case. Also added unit tests for `InferDescriptor` itself as the edge cases with that function are not covered by the descriptor roundtrip test. ACKs for top commit: furszy: ACK 74c7782 Sjors: utACK 74c7782 Tree-SHA512: ed5f63e42a2e46120245a6b0288b90d2a6912860814c6c08fe393332add1cb364dc5eca72f16980352143570aef0c07bf1a91acd294099463bd028b6ce2fe40c
bfa0bd6 test: Use pathlib over os.path bitcoin#28362 (ns-xvrn) Pull request description: In reference to issue bitcoin#28362 refactoring of functional tests to use pathlib over os.path to reduce verbosity and increase the intuitiveness of managing file access. ACKs for top commit: maflcko: re-ACK bfa0bd6 🐨 willcl-ark: ACK bfa0bd6 Tree-SHA512: fb0833c4039d09758796514e47567a93ac831cb0776ff1a7ed8299792ad132e83282ef80bea098a88ae1551d906f2a56093d3e8de240412f9080735d00d496d9
…ssumeUTXO parameters fails 2e31250 test: check that loading snapshot not matching AssumeUTXO parameters fails (Sebastian Falbesoner) Pull request description: This PR adds test coverage for the failed loading of an AssumeUTXO snapshot in case the referenced block hash doesn't match the parameters in the chainparams. Right now, I expect this would be the most common error-case for `loadtxoutset` out in the wild, as for mainnet the `m_assumeutxo_data` map is empty and this error condition would obviously always be triggered for any (otherwise valid, correctly encoded) snapshot. Note that this test-case is the simplest scenario and doesn't cover any of the TODO ideas mentioned at the top of the functional test yet. ACKs for top commit: jamesob: ACK bitcoin@2e31250 Sjors: utACK 2e31250 achow101: ACK 2e31250 Tree-SHA512: 8bcb2d525c95fbc95f87d3e978ad717d95bddb1ff67cbe7d3b06e4783f0f1ffba32b17ef451468c39c23bc1b3ef1150baa71148c145275c386f2d4822d790d39
Co-authored-by: pablomartin4btc <[email protected]>
Sometimes we are just interested in the order in which transactions would be included in a block (we want to "linearize" the transactions). Track and store this information. This doesn't change any of the bump fee calculations.
Name {low,med,high}_fee constants for reuse across file.
…nvariants fcb3069 Use CheckPackageMempoolAcceptResult for package evaluation fuzzing (Greg Sanders) 34088d6 [test util] CheckPackageMempoolAcceptResult for sanity-checking results (glozow) 651fa40 fuzz: tx_pool checks ATMP result invariants (Greg Sanders) Pull request description: Poached from bitcoin#26711 since that PR is being split apart, and modified to match current behavior. ACKs for top commit: glozow: reACK fcb3069, only whitespace changes dergoegge: ACK fcb3069 Tree-SHA512: abd687e526d8dfc8d65b3a873ece8ca35fdcbd6b0f7b93da6a723ef4e47cf85612de819e6f2b8631bdf897e1aba27cdd86f89b7bd85fc3356e74be275dcdf8cc
b5a60ab MOVEONLY: CleanupTemporaryCoins into its own function (glozow) 10c0a86 [test util] CreateValidTransaction multi-in/out, configurable feerate, signal BIP125 (glozow) 6ff647a scripted-diff: rename CheckPackage to IsWellFormedPackage (glozow) da9aceb [refactor] move package checks into helper functions (glozow) Pull request description: This is part of bitcoin#27463. It splits off the more trivial changes from bitcoin#26711 for ease of review, as requested in bitcoin#26711 (comment). - Split package sanitization in policy/packages.h into helper functions - Add some tests for its quirks (bitcoin#26711 (comment)) - Rename `CheckPackage` to `IsPackageWellFormed` - Improve the `CreateValidTransaction` unit test utility to: - Configure the target feerate and return the fee paid - Signal BIP125 on transactions to enable RBF tests - Allow the specification of multiple inputs and outputs - Move `CleanupTemporaryCoins` into its own function to be reused later without duplication ACKs for top commit: dergoegge: Code review ACK b5a60ab instagibbs: ACK b5a60ab Tree-SHA512: 39d67a5f0041e381f0d0f802a98ccffbff11e44daa3a49611189d6306b03f18613d5ff16c618898d490c97a216753e99e0db231ff14d327f92c17ae4d269cfec
… build 664c873 depends: drop -O1 workaround from arm64 apple Qt build (fanquake) Pull request description: Drop the workaround of setting optimization flags to -O1 for the `arm64-apple-darwin` builds. I no-longer see reproducibility issues when building across `x86_64` and `aarch64`: ```bash real7m21.192s user67m41.047s sys5m8.596s fedora-32gb-hel1-1 bitcoin]# find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum 62373549d2884e8ef8f46a77b9a93f64ebfc88603569e9d33b68fc67beaf2226 guix-build-664c87354f9e/output/arm64-apple-darwin/SHA256SUMS.part 597889f1908fdb67a6419177a98935b7119c637a962f03f47270893c5ba3fd6f guix-build-664c87354f9e/output/arm64-apple-darwin/bitcoin-664c87354f9e-arm64-apple-darwin-unsigned.tar.gz 289340354532a54a42b7235c831d13fdb28751c643f0fa0fc417ab195e9b5d90 guix-build-664c87354f9e/output/arm64-apple-darwin/bitcoin-664c87354f9e-arm64-apple-darwin-unsigned.zip 74f4ab3819a186d6c34ca0a4b9dda7c38fcb36bd9b22075a5d91df9bdd5df98a guix-build-664c87354f9e/output/arm64-apple-darwin/bitcoin-664c87354f9e-arm64-apple-darwin.tar.gz f0d0dad63057c7dddf6d6ccee244e7916ac0ee26b3bef8dd35f8430280043b38 guix-build-664c87354f9e/output/dist-archive/bitcoin-664c87354f9e.tar.gz fedora-32gb-hel1-1 bitcoin]# uname -m aarch64 ``` ```bash real18m10.759s user108m21.656s sys6m12.930s ubuntu-32gb-hel1-1:~/bitcoin# find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum 62373549d2884e8ef8f46a77b9a93f64ebfc88603569e9d33b68fc67beaf2226 guix-build-664c87354f9e/output/arm64-apple-darwin/SHA256SUMS.part 597889f1908fdb67a6419177a98935b7119c637a962f03f47270893c5ba3fd6f guix-build-664c87354f9e/output/arm64-apple-darwin/bitcoin-664c87354f9e-arm64-apple-darwin-unsigned.tar.gz 289340354532a54a42b7235c831d13fdb28751c643f0fa0fc417ab195e9b5d90 guix-build-664c87354f9e/output/arm64-apple-darwin/bitcoin-664c87354f9e-arm64-apple-darwin-unsigned.zip 74f4ab3819a186d6c34ca0a4b9dda7c38fcb36bd9b22075a5d91df9bdd5df98a guix-build-664c87354f9e/output/arm64-apple-darwin/bitcoin-664c87354f9e-arm64-apple-darwin.tar.gz f0d0dad63057c7dddf6d6ccee244e7916ac0ee26b3bef8dd35f8430280043b38 guix-build-664c87354f9e/output/dist-archive/bitcoin-664c87354f9e.tar.gz ubuntu-32gb-hel1-1:~/bitcoin# uname -m x86_64 ``` ACKs for top commit: hebasto: ACK 664c873. TheCharlatan: ACK 664c873 Tree-SHA512: 79527df4181eb0a0c42fe526581479abcdeba8fb09e1faf52265d697d39a8f3a3532ee3c573579b9af00b7330a401e4b6f1686636f9bac6bf9839be8381a2033
d9cc99d [test] MiniMiner::Linearize and manual construction (glozow) dfd6a37 [refactor] unify fee amounts in miniminer_tests (glozow) f4b1b24 [MiniMiner] track inclusion order and add Linearize() function (glozow) 0040759 [test] add case for MiniMiner working with negative fee txns (glozow) fe6332c [MiniMiner] make target_feerate optional (glozow) 5a83f55 [MiniMiner] allow manual construction with non-mempool txns (glozow) e3b2e63 [refactor] change MiniMinerMempoolEntry ctor to take values, update includes (glozow) 4aa98b7 [lint] update expected boost includes (glozow) Pull request description: This is part of bitcoin#27463. It splits off the `MiniMiner`-specific changes from bitcoin#26711 for ease of review, as suggested in bitcoin#26711 (comment). - Allow using `MiniMiner` on transactions that aren't in the mempool. - Make `target_feerate` param of `BuildMockTemplate` optional, meaning "don't stop building the template until all the transactions have been selected." - Add clarification for how this is different from `target_feerate=0` (bitcoin#26711 (comment)) - Track the order in which transactions are included in the template to get the "linearization order" of the transactions. - Tests Reviewers can take a look at bitcoin#26711 to see how these functions are used to linearize the `AncestorPackage` there. ACKs for top commit: TheCharlatan: ACK d9cc99d kevkevinpal: reACK [d9cc99d](bitcoin@d9cc99d) achow101: re-ACK d9cc99d Tree-SHA512: 32b80064b6679536ac573d674825c5ca0cd6245e49c2fd5eaf260dc535335a57683c74ddd7ce1f249b5b12b2683de4362a7b0f1fc0814c3b3b9f14c682665583
The underlying issue has been fixed in the image version 20231029.
…tallation" step 5bd1b8d ci: Drop no longer needed "Fix Visual Studio installation" step (Hennadii Stepanov) Pull request description: The underlying issue has been [fixed](actions/runner-images#8686) in the image version 20231029. ACKs for top commit: maflcko: lgtm ACK 5bd1b8d Tree-SHA512: d0efef3086a147d863c9b5f45ba1142c6e7cc65e47d685b2094211e58036315fb7562253b7d7172b527fa1ded4b5a86634ba7c151e761ec20fe948145aff83fe
3333f14 depends: Bump to capnproto-c++-1.0.1 (MarcoFalke) Pull request description: Reasons: * Debian is starting to ship this version in Trixie (https://packages.debian.org/trixie/capnproto), which will likely become the version shipped with Ubuntu 24.04 LTS. So testing with this version will help to find any issues before real users start to use those distro packages. * The feature is currently experimental, so bumping the version shouldn't cause any production issues. * With multiprocess begin a priority project for 27.0, it seems better to do build system changes/bumps early, rather than later, to allow for more time testing them. ACKs for top commit: TheCharlatan: Re-ACK 3333f14 fanquake: ACK 3333f14 - the response from upstream is that [if we submit a PR, they can take a look](capnproto/capnproto#1833 (comment)), so if anyone would like this to work for Windows, I'd suggest sending a patch. ryanofsky: Code review ACK 3333f14 Tree-SHA512: 7d53ad1536f042ab43dbc7847126b826e7fc76694f173c348b835fd1067b8f3dd682c5bcb4887f09ee85bab69130721cd7f8fb96b2e82053d4e28bd5c38bdc5f
Right now a wallet descriptor is converted to it's string representation (via `Descriptor::ToString`) repeatedly at different instances: - on finding a `DescriptorScriptPubKeyMan` for a given descriptor (`CWallet::GetDescriptorScriptPubKeyMan`, e.g. used by the `importdescriptors` RPC); the string representation is created once for each spkm in the wallet and at each iteration again for the searched descriptor (`DescriptorScriptPubKeyMan::HasWalletDescriptor`) - whenever `DescriptorScriptPubKeyMan::GetID()` is called, e.g. in `TopUp` or any instances where a descriptor is written to the DB to determine the database key etc. As there is no good reason to calculate a fixed descriptor's string/ID more than once, add the ID as a field to `WalletDescriptor` and calculate it immediately at initialization (or deserialization). `HasWalletDescriptor` is changed to compare the spkm's and searched descriptor's ID instead of the string to take use of that. This speeds up the functional test `wallet_miniscript.py` by a factor of 5-6x on my machine (3m30.95s on master vs. 0m38.02s on PR). The recently introduced "max-size TapMiniscript" test-case introduced a descriptor that takes 2-3 seconds to create a string representation, so the repeated calls to that were significantly hurting the performance.
…ssions 33223f9 build: Drop no longer needed MSVC warning suppressions (Hennadii Stepanov) Pull request description: ACKs for top commit: maflcko: lgtm ACK 33223f9 Tree-SHA512: 3757fce0a6900f6edc50a1b1298c5448059b100060e8a200bfc12d034852138084db12d100caefd2a19f170faa22bf4be9050094fd64f1273312541df7c81ef4
…urn all error strings 5380f05 test: bugfix CheckPackageMempoolAcceptResult return all error strings (Greg Sanders) Pull request description: Noticed on follow-up testing work https://github.com/bitcoin/bitcoin/pull/28764/files#r1382150706 ACKs for top commit: glozow: utACK 5380f05 dergoegge: utACK 5380f05 Tree-SHA512: abe122b5d702aaa0d731b45f6ba0f2f8ff9ecc14398cc087df56ee0980b5b483fc159d38ec36fe6360f82e5443673cfa80afc1c9cee6b840c95703b7dc8a8d3f
3c208cc Add offline signing tutorial (Brandon Odiwuor) Pull request description: This PR adds offline signing tutorial. Fixes bitcoin#9492 Although there currently exists tutorials on external-signer and on multisig implemented on bitcoin#24519 . The external-signer tutorial assumes a connected device and the multisig tutorial is only for multisig transactions and does not include using an offline wallet - The tutorial uses signet(instead of regtest) to be as close as possible to mainnet ACKs for top commit: achow101: ACK 3c208cc willcl-ark: ACK 3c208cc pinheadmz: ACK 3c208cc Zero-1729: ACK 3c208cc Tree-SHA512: c1686043d9e9ed440e78d219a6b18d58d62efd05bdd535e74194d8cc2db0a91e94c6c619106453120a137e47220cf3ab27af3214e861f4e5cc419a73a8704dd6
…scriptor string creation 5e6bc6d test: remove custom rpc timeout for `wallet_miniscript.py`, reorder in test_runner (Sebastian Falbesoner) f811a24 wallet: cache descriptor ID to avoid repeated descriptor string creation (Sebastian Falbesoner) Pull request description: Right now a wallet descriptor is converted to its string representation (via `Descriptor::ToString`) repeatedly at different instances: - on finding a `DescriptorScriptPubKeyMan` for a given descriptor (`CWallet::GetDescriptorScriptPubKeyMan`, e.g. used by the `importdescriptors` RPC); the string representation is created once for each spkm in the wallet and at each iteration again for the searched descriptor (`DescriptorScriptPubKeyMan::HasWalletDescriptor`) - whenever `DescriptorScriptPubKeyMan::GetID()` is called, e.g. in `TopUp` or any instances where a descriptor is written to the DB to determine the database key, also at less obvious places like `FastWalletRescanFilter` etc. As there is no good reason to calculate a fixed descriptor's string/ID more than once, add the ID as a field to `WalletDescriptor` and calculate it immediately at initialization (or deserialization). `HasWalletDescriptor` is changed to compare the spkm's and searched descriptor's ID instead of the string to take use of that. This speeds up the functional test `wallet_miniscript.py` by a factor of 5-6x on my machine (3m30.95s on master vs. 0m38.02s on PR). The recently introduced "max-size TapMiniscript" test-case introduced a descriptor that takes 2-3 seconds to create a string representation, so the repeated calls to that were significantly hurting the performance. Fixes bitcoin#28800. ACKs for top commit: Sjors: ACK 5e6bc6d S3RK: Code Review ACK 5e6bc6d achow101: ACK 5e6bc6d BrandonOdiwuor: ACK 5e6bc6d Tree-SHA512: 98b43963a5dde6055bb26cecd3b878dadd837d6226af4c84142383310495da80b3c4bd552e73b9107f2f2ff1c11f5e18060c6fd3d9e44bbd5224114c4d245c1c
5ab6419 test: randomized perturbing in feature_init (L0la L33tz) 64b80d5 test: simplify feature_init (Fabian Jahr) Pull request description: Fixes bitcoin#28603 Added suggested simplifications and implemented randomization ACKs for top commit: theStack: Light ACK 5ab6419 maflcko: lgtm ACK 5ab6419 achow101: ACK 5ab6419 Tree-SHA512: e6f43eef7f8dd12c7fccbe437cb430dc9d383825d7ab2caa0382d061f88dec6d28522e1ec78f3f58f26d35cba93512fa21e330c48d06b1d8141a16f07050af5a
Changes MiniMinerMempoolEntry order to match the order of the params elsewhere in the codebase
Refactored a variable name to be less confusing
kevkevinpal
pushed a commit
that referenced
this pull request
Dec 31, 2023
…BlockTx suppression fa9dc92 test: Add missing CBlockPolicyEstimator::processBlockTx suppression (MarcoFalke) Pull request description: Fixes bitcoin#28865 (comment) ``` # FUZZ=policy_estimator UBSAN_OPTIONS="suppressions=/root/fuzz_dir/scratch/fuzz_gen/code/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./src/test/fuzz/fuzz /tmp/crash-154b42214e70781a9c1ad72d3f2693913dcf8c06 ... policy/fees.cpp:632:27: runtime error: implicit conversion from type 'unsigned int' of value 4294574080 (32-bit, unsigned) to type 'int' changed the value to -393216 (32-bit, signed) #0 0x55cbbe10daee in CBlockPolicyEstimator::processBlockTx(unsigned int, CTxMemPoolEntry const*) src/policy/fees.cpp:632:27 #1 0x55cbbe10e361 in CBlockPolicyEstimator::processBlock(unsigned int, std::vector<CTxMemPoolEntry const*, std::allocator<CTxMemPoolEntry const*>>&) src/policy/fees.cpp:680:13 #2 0x55cbbd84af48 in policy_estimator_fuzz_target(Span<unsigned char const>)::$_1::operator()() const src/test/fuzz/policy_estimator.cpp:69:40 #3 0x55cbbd84af48 in unsigned long CallOneOf<policy_estimator_fuzz_target(Span<unsigned char const>)::$_0, policy_estimator_fuzz_target(Span<unsigned char const>)::$_1, policy_estimator_fuzz_target(Span<unsigned char const>)::$_2, policy_estimator_fuzz_target(Span<unsigned char const>)::$_3>(FuzzedDataProvider&, policy_estimator_fuzz_target(Span<unsigned char const>)::$_0, policy_estimator_fuzz_target(Span<unsigned char const>)::$_1, policy_estimator_fuzz_target(Span<unsigned char const>)::$_2, policy_estimator_fuzz_target(Span<unsigned char const>)::$_3) src/./test/fuzz/util.h:43:27 #4 0x55cbbd84af48 in policy_estimator_fuzz_target(Span<unsigned char const>) src/test/fuzz/policy_estimator.cpp:38:9 #5 0x55cbbda1cc18 in std::function<void (Span<unsigned char const>)>::operator()(Span<unsigned char const>) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9 #6 0x55cbbda1cc18 in LLVMFuzzerTestOneInput src/test/fuzz/fuzz.cpp:178:5 #7 0x55cbbd26a944 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x190e944) (BuildId: ffb89e0b86c093ca3bdeae6f85537737a4e3b42d) #8 0x55cbbd253916 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x18f7916) (BuildId: ffb89e0b86c093ca3bdeae6f85537737a4e3b42d) #9 0x55cbbd25945a in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x18fd45a) (BuildId: ffb89e0b86c093ca3bdeae6f85537737a4e3b42d) #10 0x55cbbd284026 in main (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x1928026) (BuildId: ffb89e0b86c093ca3bdeae6f85537737a4e3b42d) #11 0x7fe4aa8280cf (/lib/x86_64-linux-gnu/libc.so.6+0x280cf) (BuildId: 96ab1a8f3b2c9a2ed37c7388615e6a726d037e89) #12 0x7fe4aa828188 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x28188) (BuildId: 96ab1a8f3b2c9a2ed37c7388615e6a726d037e89) #13 0x55cbbd24e494 in _start (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x18f2494) (BuildId: ffb89e0b86c093ca3bdeae6f85537737a4e3b42d) SUMMARY: UndefinedBehaviorSanitizer: implicit-integer-sign-change policy/fees.cpp:632:27 in ``` ``` # base64 /tmp/crash-154b42214e70781a9c1ad72d3f2693913dcf8c06 AQEAAAAAADkFlVwAAQEAAAAAADkFlZVcACTDSSsP3746IAZrH48khwMAAQEB/QEALQAACwAAAAAA FgAAAAAAAQAABgAAAAAAAAAAAAAAAAAAACcQAAAAAAAAAAAAAAAAAAAAAAD6AAAAOQWVXAABAQAA AAAAOQWVlVwAIMNJKw/fvjogBmsfjySHAwABAQH9AQAtAAALAAAAAAAAAAABAAAGAAAAAAAAAAAA AAAAAAAAJxAAAAAAAAAAAAAAAAAAAAAAAPr/AAAAAAAAAAAAAAQAAAAA/wAAAAAAAAAAAAAEAAAA AAEBAeAIAVwBXAAA/jbSBvwBKABSKBwBYgEB2wAEkvXInHYAAAAAAAAAvgAAAAAA/9//6v8e/xIk MgAlAiUAOw== ACKs for top commit: fanquake: ACK fa9dc92 dergoegge: utACK fa9dc92 Tree-SHA512: 3898c17c928ecc2bcc8c7086359e9ae00da2197b4d8e10c7bf6d12415326c9bca3ef6e1d8d3b83172ccfa604ce7e7371415262ba705225f9ea4da8b1a7eb0306
kevkevinpal
pushed a commit
that referenced
this pull request
Dec 31, 2023
…tifications fuzz target fab164f fuzz: Avoid signed-integer-overflow in wallet_notifications fuzz target (MarcoFalke) Pull request description: Should avoid ``` policy/feerate.cpp:29:63: runtime error: signed integer overflow: 77600710321911316 * 149 cannot be represented in type 'int64_t' (aka 'long') #0 0x563a1775ed66 in CFeeRate::GetFee(unsigned int) const src/policy/feerate.cpp:29:63 #1 0x563a15913a69 in wallet::COutput::COutput(COutPoint const&, CTxOut const&, int, int, bool, bool, bool, long, bool, std::optional<CFeeRate>) src/./wallet/coinselection.h:91:57 #2 0x563a16fa6a6d in wallet::FetchSelectedInputs(wallet::CWallet const&, wallet::CCoinControl const&, wallet::CoinSelectionParams const&) src/wallet/spend.cpp:297:17 #3 0x563a16fc4512 in wallet::CreateTransactionInternal(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient>> const&, int, wallet::CCoinControl const&, bool) src/wallet/spend.cpp:1105:33 #4 0x563a16fbec74 in wallet::CreateTransaction(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient>> const&, int, wallet::CCoinControl const&, bool) src/wallet/spend.cpp:1291:16 #5 0x563a16fcf6df in wallet::FundTransaction(wallet::CWallet&, CMutableTransaction&, long&, int&, bilingual_str&, bool, std::set<int, std::less<int>, std::allocator<int>> const&, wallet::CCoinControl) src/wallet/spend.cpp:1361:16 #6 0x563a1597b7b9 in wallet::(anonymous namespace)::FuzzedWallet::FundTx(FuzzedDataProvider&, CMutableTransaction) src/wallet/test/fuzz/notifications.cpp:162:15 #7 0x563a15958240 in wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>)::$_0::operator()() const src/wallet/test/fuzz/notifications.cpp:228:23 #8 0x563a15958240 in unsigned long CallOneOf<wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>)::$_0, wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>)::$_1>(FuzzedDataProvider&, wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>)::$_0, wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>)::$_1) src/./test/fuzz/util.h:43:27 #9 0x563a15958240 in wallet::(anonymous namespace)::wallet_notifications_fuzz_target(Span<unsigned char const>) src/wallet/test/fuzz/notifications.cpp:196:9 #10 0x563a15fdef0c in std::function<void (Span<unsigned char const>)>::operator()(Span<unsigned char const>) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9 #11 0x563a15fdef0c in LLVMFuzzerTestOneInput src/test/fuzz/fuzz.cpp:178:5 #12 0x563a158032a4 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x19822a4) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) #13 0x563a15802999 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1981999) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) #14 0x563a15804586 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1983586) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) bitcoin#15 0x563a15804aa7 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1983aa7) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) bitcoin#16 0x563a157f21fb in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x19711fb) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) bitcoin#17 0x563a1581c766 in main (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x199b766) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) bitcoin#18 0x7f499e17b0cf (/lib/x86_64-linux-gnu/libc.so.6+0x280cf) (BuildId: 96ab1a8f3b2c9a2ed37c7388615e6a726d037e89) bitcoin#19 0x7f499e17b188 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x28188) (BuildId: 96ab1a8f3b2c9a2ed37c7388615e6a726d037e89) bitcoin#20 0x563a157e70c4 in _start (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x19660c4) (BuildId: 8acb42ad599d7f6d25b6f93e18fd564d80df7c06) SUMMARY: UndefinedBehaviorSanitizer: signed-integer-overflow policy/feerate.cpp:29:63 in MS: 0 ; base unit: 0000000000000000000000000000000000000000 0x3f,0x0,0x2f,0x5f,0x5f,0x5f,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0x7d,0xff,0xff,0xff,0xff,0xff,0x53,0xff,0xff,0xff,0xff,0xff,0x0,0x0,0x0,0x0,0x0,0x0,0x13,0x5e,0x5f,0x5f,0x8,0x25,0x0,0x5f,0x5f,0x5f,0x5f,0x5f,0x5f,0x8,0x25,0xca,0x7f,0x5f,0x5f,0x5f,0x13,0x13,0x5f,0x5f,0x5f,0x2,0xdb,0xca,0x0,0x0,0xe7,0xe6,0x66,0x65,0x0,0x0,0x0,0x0,0x44,0x3f,0xa,0xa,0xff,0xff,0xff,0xff,0xff,0x61,0x76,0x6f,0x69,0x0,0xb5,0x15, ?\000/___}}}}}}}}}}}}}}}}}}}}\377\377\377\377\377S\377\377\377\377\377\000\000\000\000\000\000\023^__\010%\000______\010%\312\177___\023\023___\002\333\312\000\000\347\346fe\000\000\000\000D?\012\012\377\377\377\377\377avoi\000\265\025 artifact_prefix='./'; Test unit written to ./crash-4d3bac8a64d4e58b2f0943e6d28e6e1f16328d7d Base64: PwAvX19ffX19fX19fX19fX19fX19fX19fX3//////1P//////wAAAAAAABNeX18IJQBfX19fX18IJcp/X19fExNfX18C28oAAOfmZmUAAAAARD8KCv//////YXZvaQC1FQ== ACKs for top commit: dergoegge: ACK fab164f brunoerg: ACK fab164f Tree-SHA512: f416828f4394aa7303ee437f141e9bbd23c0e0f1b830e4ef3932338858249ba68a811b9837c5b7ad8c6ab871b6354996434183597c1a910a8d8e8d829693e4b2
kevkevinpal
pushed a commit
that referenced
this pull request
Aug 1, 2024
The previous commit added a test which would fail the
unsigned-integer-overflow sanitizer. The warning is harmless and can be
triggered on any commit, since the code was introduced.
For reference, the warning would happen when the separator `-` was not
present.
For example:
GET /rest/getutxos/6a297bfa5cb8dd976ab0207a767d6cbfaa5e876f30081127ec8674c8c52b16c0_+1.json
would result in:
rest.cpp:792:77: runtime error: unsigned integer overflow: 18446744073709551615 + 1 cannot be represented in type 'size_type' (aka 'unsigned long')
#0 0x55ad42c16931 in rest_getutxos(std::any const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&) src/rest.cpp:792:77
#1 0x55ad4319e3c0 in std::function<bool (HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&)>::operator()(HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9
#2 0x55ad4319e3c0 in HTTPWorkItem::operator()() src/httpserver.cpp:59:9
#3 0x55ad431a3eea in WorkQueue<HTTPClosure>::Run() src/httpserver.cpp:114:13
#4 0x55ad4318f961 in HTTPWorkQueueRun(WorkQueue<HTTPClosure>*, int) src/httpserver.cpp:403:12
#5 0x7f078ebcbbb3 (/lib/x86_64-linux-gnu/libstdc++.so.6+0xeabb3) (BuildId: 40b9b0d17fdeebfb57331304da2b7f85e1396ef2)
#6 0x55ad4277e01c in asan_thread_start(void*) asan_interceptors.cpp.o
#7 0x7f078e840a93 (/lib/x86_64-linux-gnu/libc.so.6+0x9ca93) (BuildId: 08134323d00289185684a4cd177d202f39c2a5f3)
#8 0x7f078e8cdc3b (/lib/x86_64-linux-gnu/libc.so.6+0x129c3b) (BuildId: 08134323d00289185684a4cd177d202f39c2a5f3)
SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow rest.cpp:792:77
kevkevinpal
pushed a commit
that referenced
this pull request
Nov 12, 2024
…et_create_transaction 5a26cf7 fuzz: fix `implicit-integer-sign-change` in wallet_create_transaction (brunoerg) Pull request description: This PR limites the value of `m_confirm_target` to avoid `implicit-integer-sign-change`: ``` /ci_container_base/src/wallet/fees.cpp:58:58: runtime error: implicit conversion from type 'unsigned int' of value 4294967292 (32-bit, unsigned) to type 'int' changed the value to -4 (32-bit, signed) #0 0x55b6fd26c021 in wallet::GetMinimumFeeRate(wallet::CWallet const&, wallet::CCoinControl const&, FeeCalculation*) ci/scratch/build-x86_64-pc-linux-gnu/src/wallet/./src/wallet/fees.cpp:58:58 #1 0x55b6fd3ef5ca in wallet::CreateTransactionInternal(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient>> const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) ci/scratch/build-x86_64-pc-linux-gnu/src/wallet/./src/wallet/spend.cpp:1101:49 #2 0x55b6fd3ebea5 in wallet::CreateTransaction(wallet::CWallet&, std::vector<wallet::CRecipient, std::allocator<wallet::CRecipient>> const&, std::optional<unsigned int>, wallet::CCoinControl const&, bool) ci/scratch/build-x86_64-pc-linux-gnu/src/wallet/./src/wallet/spend.cpp:1382:16 #3 0x55b6fccbc154 in wallet::(anonymous namespace)::wallet_create_transaction_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>) ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/./src/wallet/test/fuzz/spend.cpp:99:11 #4 0x55b6fccda45d in std::function<void (std::span<unsigned char const, 18446744073709551615ul>)>::operator()(std::span<unsigned char const, 18446744073709551615ul>) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9 #5 0x55b6fccda45d in LLVMFuzzerTestOneInput ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/util/./src/test/fuzz/fuzz.cpp:211:5 #6 0x55b6fc368484 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1c8a484) (BuildId: d11f8692b05f02b5a14b6e7579598b426e3144c5) #7 0x55b6fc367b79 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1c89b79) (BuildId: d11f8692b05f02b5a14b6e7579598b426e3144c5) #8 0x55b6fc369796 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1c8b796) (BuildId: d11f8692b05f02b5a14b6e7579598b426e3144c5) #9 0x55b6fc369ca7 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1c8bca7) (BuildId: d11f8692b05f02b5a14b6e7579598b426e3144c5) #10 0x55b6fc35719f in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1c7919f) (BuildId: d11f8692b05f02b5a14b6e7579598b426e3144c5) #11 0x55b6fc381826 in main (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1ca3826) (BuildId: d11f8692b05f02b5a14b6e7579598b426e3144c5) #12 0x7f934c6661c9 (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 6d64b17fbac799e68da7ebd9985ddf9b5cb375e6) #13 0x7f934c66628a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 6d64b17fbac799e68da7ebd9985ddf9b5cb375e6) #14 0x55b6fc34c184 in _start (/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x1c6e184) (BuildId: d11f8692b05f02b5a14b6e7579598b426e3144c5) SUMMARY: UndefinedBehaviorSanitizer: implicit-integer-sign-change /ci_container_base/src/wallet/fees.cpp:58:58 MS: 0 ; base unit: 0000000000000000000000000000000000000000 0x2e,0x1,0xb0,0xb8,0x0,0xff,0xff,0xff,0xff,0x60,0x14,0x22,0xff,0xff,0xff,0xff,0xff,0xfd,0xff,0xff,0xff,0xff,0xff,0x7e,0xf9,0x41,0x8,0x2b,0x17,0x58,0xb,0x17,0xfc,0xff,0xff,0xff,0xff,0xff,0xff,0x7e,0xf9,0x41,0x8,0x2b,0x17,0x58,0xb, .\001\260\270\000\377\377\377\377`\024\"\377\377\377\377\377\375\377\377\377\377\377~\371A\010+\027X\013\027\374\377\377\377\377\377\377~\371A\010+\027X\013 artifact_prefix='./'; Test unit written to ./crash-5627f57ffba7568a500f8379f62c3338978b43f2 Base64: LgGwuAD/////YBQi///////9//////9++UEIKxdYCxf8////////fvlBCCsXWAs= ``` ACKs for top commit: maflcko: lgtm ACK 5a26cf7 dergoegge: utACK 5a26cf7 Tree-SHA512: a1b129d81d42350cf85ff6fb95cd6982b6aac88467a526ee4b3c9b3313af2f7952c5dfa9886f455756faba399d8356b6c318d7ab2d6318e08fea838bee90b2fe
kevkevinpal
pushed a commit
that referenced
this pull request
Nov 27, 2024
…-overflow
The previous commit added a test which would fail the
unsigned-integer-overflow sanitizer. The warning is harmless and can be
triggered on any commit, since the code was introduced.
For reference, the warning would happen when the separator `-` was not
present.
For example:
GET /rest/getutxos/6a297bfa5cb8dd976ab0207a767d6cbfaa5e876f30081127ec8674c8c52b16c0_+1.json
would result in:
rest.cpp:792:77: runtime error: unsigned integer overflow: 18446744073709551615 + 1 cannot be represented in type 'size_type' (aka 'unsigned long')
#0 0x55ad42c16931 in rest_getutxos(std::any const&, HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&) src/rest.cpp:792:77
#1 0x55ad4319e3c0 in std::function<bool (HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&)>::operator()(HTTPRequest*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9
#2 0x55ad4319e3c0 in HTTPWorkItem::operator()() src/httpserver.cpp:59:9
#3 0x55ad431a3eea in WorkQueue<HTTPClosure>::Run() src/httpserver.cpp:114:13
#4 0x55ad4318f961 in HTTPWorkQueueRun(WorkQueue<HTTPClosure>*, int) src/httpserver.cpp:403:12
#5 0x7f078ebcbbb3 (/lib/x86_64-linux-gnu/libstdc++.so.6+0xeabb3) (BuildId: 40b9b0d17fdeebfb57331304da2b7f85e1396ef2)
#6 0x55ad4277e01c in asan_thread_start(void*) asan_interceptors.cpp.o
#7 0x7f078e840a93 (/lib/x86_64-linux-gnu/libc.so.6+0x9ca93) (BuildId: 08134323d00289185684a4cd177d202f39c2a5f3)
#8 0x7f078e8cdc3b (/lib/x86_64-linux-gnu/libc.so.6+0x129c3b) (BuildId: 08134323d00289185684a4cd177d202f39c2a5f3)
SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow rest.cpp:792:77
Github-Pull: bitcoin#30444
Rebased-From: fac932b
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.
No description provided.