📝 ajtowns opened a pull request: "doc: Update comments for AreInputsStandard to match code"
(https://github.com/bitcoin/bitcoin/pull/32129)
The comment about extra data stuffed in scriptSigs was introduced in #4365 which introduced `ScriptSigArgsExpected()`, and became incorrect after #7387 / #7453 (checks are now performed by `SCRIPT_VERIFY_CLEANSTACK` during script validation and `IsPushOnly()` in `IsStandardTx()`). Drops the details on what a p2sh with many checksigs would look like, which was already done in #4365, but only for main.cpp not the duplicated comment in main.h, which was merged into policy/policy.cpp in #6335 and la
...
(https://github.com/bitcoin/bitcoin/pull/32129)
The comment about extra data stuffed in scriptSigs was introduced in #4365 which introduced `ScriptSigArgsExpected()`, and became incorrect after #7387 / #7453 (checks are now performed by `SCRIPT_VERIFY_CLEANSTACK` during script validation and `IsPushOnly()` in `IsStandardTx()`). Drops the details on what a p2sh with many checksigs would look like, which was already done in #4365, but only for main.cpp not the duplicated comment in main.h, which was merged into policy/policy.cpp in #6335 and la
...
💬 Sjors commented on pull request "Rust tool to import bip39 mnemonic":
(https://github.com/bitcoin/bitcoin/pull/32115#issuecomment-2747437410)
> closer where the majority of its (implicit) code sits
The `importdescriptors` RPC is highly specific to Bitcoin Core, and all that code lives here.
The script itself seems trivial to maintain, especially if someone who actually knows Rust cleans it up a bit.
The problem is in the dependencies.
> several imported rust-bitcoin dependencies
We only need a small (?) subset of this, but I'm not familiar enough with Rust to constrain the imports. And perhaps a few changes to rust-bitc
...
(https://github.com/bitcoin/bitcoin/pull/32115#issuecomment-2747437410)
> closer where the majority of its (implicit) code sits
The `importdescriptors` RPC is highly specific to Bitcoin Core, and all that code lives here.
The script itself seems trivial to maintain, especially if someone who actually knows Rust cleans it up a bit.
The problem is in the dependencies.
> several imported rust-bitcoin dependencies
We only need a small (?) subset of this, but I'm not familiar enough with Rust to constrain the imports. And perhaps a few changes to rust-bitc
...
💬 dergoegge commented on pull request "cmake: Avoid fuzzer "multiple definition of `main'" errors":
(https://github.com/bitcoin/bitcoin/pull/31992#issuecomment-2747463640)
Re: https://github.com/bitcoin/bitcoin/pull/31992#issuecomment-2717920553
Thank you for explaining.
I haven't tested this but looking at the diff here I don't think this will work for all fuzzing engines, as it only works around the error for libFuzzer. Oss-fuzz for example uses `FUZZ_LIBS` to link against the afl++ libfuzzer driver (i.e. `FUZZ_LIBS=/usr/lib/libFuzzingEngine.a`). This is just one example and it is quite useful to allow linking against pretty much anything here.
Could we
...
(https://github.com/bitcoin/bitcoin/pull/31992#issuecomment-2747463640)
Re: https://github.com/bitcoin/bitcoin/pull/31992#issuecomment-2717920553
Thank you for explaining.
I haven't tested this but looking at the diff here I don't think this will work for all fuzzing engines, as it only works around the error for libFuzzer. Oss-fuzz for example uses `FUZZ_LIBS` to link against the afl++ libfuzzer driver (i.e. `FUZZ_LIBS=/usr/lib/libFuzzingEngine.a`). This is just one example and it is quite useful to allow linking against pretty much anything here.
Could we
...
💬 maflcko commented on pull request "Rust tool to import bip39 mnemonic":
(https://github.com/bitcoin/bitcoin/pull/32115#discussion_r2009802186)
> The mnemonic.
Unless I am missing something, that'll lead to loss of funds, because `importdescriptors` does not fail if there are already descriptors in the wallet, and it doesn't guarantee that all prior descriptors are inactive. So a users could (after they ran through the steps here) silently and accidentally use a prior active descriptor and then fail to recover the funds there, because they only backed up the mnemonic.
> If the original wallet uses different derivation
...
(https://github.com/bitcoin/bitcoin/pull/32115#discussion_r2009802186)
> The mnemonic.
Unless I am missing something, that'll lead to loss of funds, because `importdescriptors` does not fail if there are already descriptors in the wallet, and it doesn't guarantee that all prior descriptors are inactive. So a users could (after they ran through the steps here) silently and accidentally use a prior active descriptor and then fail to recover the funds there, because they only backed up the mnemonic.
> If the original wallet uses different derivation
...
🤔 rkrux reviewed a pull request: "test: add missing segwitv1 test cases to `script_standard_tests`"
(https://github.com/bitcoin/bitcoin/pull/31340#pullrequestreview-2709716923)
Re: https://github.com/bitcoin/bitcoin/pull/31340#issuecomment-2746468990
I'm ok with changing the scoping of `ANCHOR_BYTES` in a follow-up. IMHO it can be added in `CScript` that can be further be used by `struct PayToAnchor` as this file `addresstype.h` already uses `CScript`, otherwise in order to deduplicate `4e73`, `ANCHOR_BYTES` from `struct PayToAnchor` would need to be used in `CScript::IsPayToAnchor` that would create a circular dependency.
ACK 8284229a28c09c585356dcf7e4bddbc8f2a
...
(https://github.com/bitcoin/bitcoin/pull/31340#pullrequestreview-2709716923)
Re: https://github.com/bitcoin/bitcoin/pull/31340#issuecomment-2746468990
I'm ok with changing the scoping of `ANCHOR_BYTES` in a follow-up. IMHO it can be added in `CScript` that can be further be used by `struct PayToAnchor` as this file `addresstype.h` already uses `CScript`, otherwise in order to deduplicate `4e73`, `ANCHOR_BYTES` from `struct PayToAnchor` would need to be used in `CScript::IsPayToAnchor` that would create a circular dependency.
ACK 8284229a28c09c585356dcf7e4bddbc8f2a
...
💬 rkrux commented on pull request "test: add missing segwitv1 test cases to `script_standard_tests`":
(https://github.com/bitcoin/bitcoin/pull/31340#discussion_r2009797523)
What's exactly the point of having `Assume` here? We pass hardcoded values right in the call here and immediately test the correctness of those hardcoded values?
(https://github.com/bitcoin/bitcoin/pull/31340#discussion_r2009797523)
What's exactly the point of having `Assume` here? We pass hardcoded values right in the call here and immediately test the correctness of those hardcoded values?
💬 dergoegge commented on pull request "Rust tool to import bip39 mnemonic":
(https://github.com/bitcoin/bitcoin/pull/32115#issuecomment-2747518535)
Concept NACK
Shipping rust code in `share/` that users have to compile themselves seems like the wrong approach. Imo, bip39 should either be supported properly (available to all users without jumping through hoops) or not at all.
The amount of users that would compile this themselves is marginal making the upside of this pretty small. Technical users like that would have no trouble finding this in the e.g. `rust-bitcoin` examples or elsewhere.
(https://github.com/bitcoin/bitcoin/pull/32115#issuecomment-2747518535)
Concept NACK
Shipping rust code in `share/` that users have to compile themselves seems like the wrong approach. Imo, bip39 should either be supported properly (available to all users without jumping through hoops) or not at all.
The amount of users that would compile this themselves is marginal making the upside of this pretty small. Technical users like that would have no trouble finding this in the e.g. `rust-bitcoin` examples or elsewhere.
👍 dergoegge approved a pull request: "fuzz: Fix off-by-one in package_rbf target"
(https://github.com/bitcoin/bitcoin/pull/32122#pullrequestreview-2709789287)
utACK fa9d9a33420c2db35e9ee7eebe5d6e475e26a8e8
This was caught in the qa-assets CI because we're using a hardened libc++ build? Trying to understand why I haven't seen this in my fuzzing.
(https://github.com/bitcoin/bitcoin/pull/32122#pullrequestreview-2709789287)
utACK fa9d9a33420c2db35e9ee7eebe5d6e475e26a8e8
This was caught in the qa-assets CI because we're using a hardened libc++ build? Trying to understand why I haven't seen this in my fuzzing.
💬 maflcko commented on pull request "fuzz: Fix off-by-one in package_rbf target":
(https://github.com/bitcoin/bitcoin/pull/32122#issuecomment-2747581208)
> This was caught in the qa-assets CI because we're using a hardened libc++ build? Trying to understand why I haven't seen this in my fuzzing.
Yes. This particular one doesn't require an ABI breaking build, so I think you should be able to find it with any of (fast, extensive, debug) from https://libcxx.llvm.org/Hardening.html#mapping-between-the-hardening-modes-and-the-assertion-categories. Same for GCC: It should be found by any of (ASSERTIONS, DEBUG, DEBUG_PEDANTIC). However, I haven't tri
...
(https://github.com/bitcoin/bitcoin/pull/32122#issuecomment-2747581208)
> This was caught in the qa-assets CI because we're using a hardened libc++ build? Trying to understand why I haven't seen this in my fuzzing.
Yes. This particular one doesn't require an ABI breaking build, so I think you should be able to find it with any of (fast, extensive, debug) from https://libcxx.llvm.org/Hardening.html#mapping-between-the-hardening-modes-and-the-assertion-categories. Same for GCC: It should be found by any of (ASSERTIONS, DEBUG, DEBUG_PEDANTIC). However, I haven't tri
...
⚠️ l0rinc opened an issue: "RFC: Macro Regression Test Suite for Historical Reorgs"
(https://github.com/bitcoin/bitcoin/issues/32130)
### Context and Motivation
We've recently modified caching behavior ([optimizations](https://github.com/bitcoin/bitcoin/pull/28280), [refactors](https://github.com/bitcoin/bitcoin/pull/30906), [new features](https://github.com/bitcoin/bitcoin/pull/31553), [calculating additional metrics](https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1925501920)), and a common concern is often: "Sweet, but have you tested it via a reorg?!"
We already have tests covering basic reorg scenarios ([featu
...
(https://github.com/bitcoin/bitcoin/issues/32130)
### Context and Motivation
We've recently modified caching behavior ([optimizations](https://github.com/bitcoin/bitcoin/pull/28280), [refactors](https://github.com/bitcoin/bitcoin/pull/30906), [new features](https://github.com/bitcoin/bitcoin/pull/31553), [calculating additional metrics](https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1925501920)), and a common concern is often: "Sweet, but have you tested it via a reorg?!"
We already have tests covering basic reorg scenarios ([featu
...
💬 Sjors commented on pull request "Rust tool to import bip39 mnemonic":
(https://github.com/bitcoin/bitcoin/pull/32115#discussion_r2009883079)
I assumed you were talking about losing from _from the imported wallet_, but I think you're talking about funds in a pre-existing Bitcoin Core wallet.
> does not fail if there are already descriptors in the wallet
The instructions say that the wallet should be blank. Perhaps the script should make the RPC calls so that it can actually verify this, but using RPC increases complexity and presumably requires importing network or shell code.
(https://github.com/bitcoin/bitcoin/pull/32115#discussion_r2009883079)
I assumed you were talking about losing from _from the imported wallet_, but I think you're talking about funds in a pre-existing Bitcoin Core wallet.
> does not fail if there are already descriptors in the wallet
The instructions say that the wallet should be blank. Perhaps the script should make the RPC calls so that it can actually verify this, but using RPC increases complexity and presumably requires importing network or shell code.
✅ Sjors closed a pull request: "Rust tool to import bip39 mnemonic"
(https://github.com/bitcoin/bitcoin/pull/32115)
(https://github.com/bitcoin/bitcoin/pull/32115)
💬 Sjors commented on issue "support BIP39 mnemonic in descriptors":
(https://github.com/bitcoin/bitcoin/issues/19151#issuecomment-2747613797)
Every attempt to do this has been met concept NACKs or lack of review. Maybe we should just decide to not do this and close?
(https://github.com/bitcoin/bitcoin/issues/19151#issuecomment-2747613797)
Every attempt to do this has been met concept NACKs or lack of review. Maybe we should just decide to not do this and close?
🤔 rkrux reviewed a pull request: "doc: clarify the documentation of `Assume` assertion"
(https://github.com/bitcoin/bitcoin/pull/32100#pullrequestreview-2709900914)
Concept ACK a880d1bf87817b1e6606c971cbfe98382898a0be
I find myself agreeing with the points mentioned in this comment https://github.com/bitcoin/bitcoin/pull/32100#discussion_r2008737842, there is some redundancy in the verbiage currently that can be removed.
(https://github.com/bitcoin/bitcoin/pull/32100#pullrequestreview-2709900914)
Concept ACK a880d1bf87817b1e6606c971cbfe98382898a0be
I find myself agreeing with the points mentioned in this comment https://github.com/bitcoin/bitcoin/pull/32100#discussion_r2008737842, there is some redundancy in the verbiage currently that can be removed.
💬 ajtowns commented on pull request "fuzz: enable running fuzz test cases in Debug mode":
(https://github.com/bitcoin/bitcoin/pull/32113#discussion_r2009964213)
Personally, I'd prefer to run the fuzz corpora against the actual logic not the stubbed out logic, so I'd personally rather run with `FUZZ_NONDETERMINISM` set. I don't see why reviewers would have to take this into account at all; it's just an alternate way to exercise test code, it has no effect on production code.
(https://github.com/bitcoin/bitcoin/pull/32113#discussion_r2009964213)
Personally, I'd prefer to run the fuzz corpora against the actual logic not the stubbed out logic, so I'd personally rather run with `FUZZ_NONDETERMINISM` set. I don't see why reviewers would have to take this into account at all; it's just an alternate way to exercise test code, it has no effect on production code.
⚠️ l0rinc opened an issue: "RFC: Compact Block Reconstruction Macro Benchmark Suite"
(https://github.com/bitcoin/bitcoin/issues/32131)
### Context and Motivation
Compact blocks significantly improve block propagation efficiency by reducing bandwidth usage and latency during transmission. Precise benchmarking of compact block reconstruction performance is crucial for detecting regressions or improvements across releases, especially when modifying related code paths affecting mempool behavior or block relay performance.
Recent [analysis by `B10C`](https://delvingbitcoin.org/t/stats-on-compact-block-reconstructions/1052/12) high
...
(https://github.com/bitcoin/bitcoin/issues/32131)
### Context and Motivation
Compact blocks significantly improve block propagation efficiency by reducing bandwidth usage and latency during transmission. Precise benchmarking of compact block reconstruction performance is crucial for detecting regressions or improvements across releases, especially when modifying related code paths affecting mempool behavior or block relay performance.
Recent [analysis by `B10C`](https://delvingbitcoin.org/t/stats-on-compact-block-reconstructions/1052/12) high
...
💬 saikiran57 commented on pull request "wallet: removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#issuecomment-2747802273)
Hi @maflcko I've updated the steps to reproduces the issue. Have a look into the the experiment files for the behavior (I've mimicked the code) https://github.com/saikiran57/cpp17/blob/develop/experiment/map_with_high_load.cpp
(https://github.com/bitcoin/bitcoin/pull/32023#issuecomment-2747802273)
Hi @maflcko I've updated the steps to reproduces the issue. Have a look into the the experiment files for the behavior (I've mimicked the code) https://github.com/saikiran57/cpp17/blob/develop/experiment/map_with_high_load.cpp
💬 maflcko commented on pull request "fuzz: enable running fuzz test cases in Debug mode":
(https://github.com/bitcoin/bitcoin/pull/32113#issuecomment-2747808769)
lgtm ACK 1718da848a0a7591f0eab086e159f1e9e0f2c59b
This should also address the concern by @ryanofsky in https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2451865832 to some extent IIUC. Of course, it is still impossible to execute a fuzz test case normally in a release-mode non-fuzz build, but this pull offers one workaround:
* Picking a debug-mode build to build all binaries and manually overriding the cxx-flags to increase the optimization level to the level of a release-mode bu
...
(https://github.com/bitcoin/bitcoin/pull/32113#issuecomment-2747808769)
lgtm ACK 1718da848a0a7591f0eab086e159f1e9e0f2c59b
This should also address the concern by @ryanofsky in https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2451865832 to some extent IIUC. Of course, it is still impossible to execute a fuzz test case normally in a release-mode non-fuzz build, but this pull offers one workaround:
* Picking a debug-mode build to build all binaries and manually overriding the cxx-flags to increase the optimization level to the level of a release-mode bu
...
💬 Eunovo commented on pull request "descriptors: taproot partial descriptors":
(https://github.com/bitcoin/bitcoin/pull/30243#issuecomment-2747824269)
Rebased on master https://github.com/bitcoin/bitcoin/commit/770d39a37652d40885533fecce37e9f71cc0d051
- changed `Span` to `std::span`
(https://github.com/bitcoin/bitcoin/pull/30243#issuecomment-2747824269)
Rebased on master https://github.com/bitcoin/bitcoin/commit/770d39a37652d40885533fecce37e9f71cc0d051
- changed `Span` to `std::span`
💬 Eunovo commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#issuecomment-2747830190)
Rebased on master https://github.com/bitcoin/bitcoin/commit/770d39a37652d40885533fecce37e9f71cc0d051
- fix failing functional test by changing `self.send_message` to `self.send_without_ping` due to https://github.com/bitcoin/bitcoin/commit/fa9cf38ab666b50b0d8e82cb17f9e5a8a613547d
(https://github.com/bitcoin/bitcoin/pull/31615#issuecomment-2747830190)
Rebased on master https://github.com/bitcoin/bitcoin/commit/770d39a37652d40885533fecce37e9f71cc0d051
- fix failing functional test by changing `self.send_message` to `self.send_without_ping` due to https://github.com/bitcoin/bitcoin/commit/fa9cf38ab666b50b0d8e82cb17f9e5a8a613547d