💬 maflcko commented on pull request "fuzz: Fix off-by-one in package_rbf target":
(https://github.com/bitcoin/bitcoin/pull/32122#issuecomment-2747190493)
> why not just modify `initialize_package_rbf` to use this statement

Why would that be better? Once an off-by-two is added in a future patch, the fuzz target once again will crash, whereas with the patch in this pull, it will work fine?

💬 Sjors commented on pull request "signet: omit commitment for some trivial challenges":
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r2009667758)
It seems to be caused by #31866, because the test passes when I revert it.

💬 maflcko commented on pull request "signet: omit commitment for some trivial challenges":
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r2009685207)
You'll have to use `node.binaries.rpc_argv()` instead here

💬 Sjors commented on pull request "signet: omit commitment for some trivial challenges":
(https://github.com/bitcoin/bitcoin/pull/29032#issuecomment-2747275625)
Rebased after #31866 to use `--cli={shlex.join(rpc_argv)}` in `mine_block_manual`.

💬 maflcko commented on pull request "fuzz: enable running fuzz test cases in Debug mode":
(https://github.com/bitcoin/bitcoin/pull/32113#discussion_r2009712764)
Is there a use-case for FUZZ_NONDETERMINISM? IIRC the goal of this change was to achieve fuzz determinism, so adding non-determinism could be left for a follow-up? I'd even argue that it may be better to not add at all, because it is such an edge case that no one will be using it. However, it existing, implies that reviewers will have to take it into account every time, which doesn't seem worth it.

If you remove it, you can also move the `assert(EnableFuzzDeterminism());` before `Assert(!g_te
...

💬 maflcko commented on issue "bitcoind crash with corrupt wallet.dat":
(https://github.com/bitcoin/bitcoin/issues/32124#issuecomment-2747328830)
> Not sure if it is relevant to [#32111](https://github.com/bitcoin/bitcoin/issues/32111).

32111 is about migrating (corrupt) bdb wallets (likely obtained from fuzzing), with exact steps to reproduce and the crash being due to `Assertion m_wallet_flags == 0 failed`. Whereas this issue is about loading (?) a corrupt (?) sqlite wallet, without steps to reproduce. Also, it is unclear how the two sqlite errors interact and if the second one is due to another dangling process on the system, or not.

...

🤔 rkrux reviewed a pull request: "contrib: refactor: dedup deserialization routines in utxo-to-sqlite script"
(https://github.com/bitcoin/bitcoin/pull/32116#pullrequestreview-2709590637)
utACK f535b2fe63255175f7a35882599f9b6b83ac25f1

I did realise there'd be some duplication when I previously supported adding and using these deserialisation functions in https://github.com/bitcoin/bitcoin/pull/31907#discussion_r1965270838, good that you found a way to get rid of the recently introduced duplication.

💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r2009729335)
I improved the commit message.

💬 Sjors commented on pull request "rpc: add optional blockhash to waitfornewblock, unhide wait methods in help":
(https://github.com/bitcoin/bitcoin/pull/30635#issuecomment-2747341913)
My latest change to #31785 only changed the commit message, so I'm not rebasing this yet.

💬 hodlinator commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2747360632)
### unique_ptr

Here is a branch implementing another possible variant, using `unique_ptr` as suggested by @purpleKarrot in https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2711842250:
https://github.com/bitcoin/bitcoin/compare/master...hodlinator:bitcoin:pr/32015_unique_ptr

#### Pros

- It keeps the `CNode` ownership/lifetime under control of `CConnman::m_nodes`/`m_nodes_disconnected`. This can be considered an advancement over the unclear ownership/lifetime of `shared_ptr` ([
...

💬 maflcko commented on issue "Failed transactions on importing mempool":
(https://github.com/bitcoin/bitcoin/issues/32125#issuecomment-2747360730)
> The nodes I test are running on my local network so I guess all the transactions are importable, but the 10 failed could be 5 packages of 2.

Can you add exact steps to reproduce, please?

The mempool acceptance depends on the chain state (the best block hash of your node at the time of the mempool import), as well as the mempool file contents.

Without any exact info, my guess would be that the 10 failures are due to transactions already confirmed (either themselves or a replacement of themse
...

💬 Sjors commented on pull request "Rust tool to import bip39 mnemonic":
(https://github.com/bitcoin/bitcoin/pull/32115#discussion_r2009752779)
The mnemonic. Some wallets let you see the mnemonic, others don't (e.g. Ledger only shows it on first use, Bitkey doesn't show it all). But we can't show it, because the master hd key is a hash of the seed, so there's no way back.

If the original wallet uses different derivation paths then Bitcoin Core won't find funds, but they're not lost. Using the same key material between two wallets is a bad idea in general, so we could advice against that.

The docs should probably also mention that
...

📝 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
...

💬 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
...

💬 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
...

💬 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
...

🤔 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
...

💬 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?

💬 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.

👍 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.