💬 sipa commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#discussion_r2219698256)
Use a scripted diff?

💬 sipa commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#discussion_r2219704726)
That's a lot of fuzzer bytes (32) for very little gain I think. Since it's already fed through a cryptographic RNG anyway, I don't think consuming bytes from the fuzzer has any advantage over using the harness' `rng` (which is already seeded by fuzz input anyway).

💬 sipa commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#discussion_r2219589388)
Hmm, this means that anyone with a `bitcoin.conf` that lists `maxorphantxs=N` will now get an error. An alternative could be to leave the option in for a while, but make it just emit a warning. Unsure if that's necessary; I doubt many people configure this, but I don't remember how we've dealt with similar changes in recent history.

💬 sipa commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#discussion_r2219699199)
Use a scripted-diff?

💬 l0rinc commented on pull request "p2p: rename GetAddresses -> GetAddressesUncached":
(https://github.com/bitcoin/bitcoin/pull/32994#discussion_r2219731173)
Actually, first seeing `Uncached` in a name implies to me that it's safer, since it doesn't have side-effects, doesn't pollute the context, it cannot blow up, etc - I don't think it achieves scaring people away.
In other cases the suffix was "Unsafe" if it's important to signal that the user should pay extra attention.

💬 Zeegaths commented on pull request "docs: adds correct updated documentation links":
(https://github.com/bitcoin/bitcoin/pull/32699#issuecomment-3097660338)
thanks for pointing that out. all good now

On Mon, 21 Jul 2025 at 17:19, maflcko ***@***.***> wrote:

> *maflcko* left a comment (bitcoin/bitcoin#32699)
> <https://github.com/bitcoin/bitcoin/pull/32699#issuecomment-3096984862>
>
> No, you haven't replied/addressed the feedback from June: #32699 (comment)
> <https://github.com/bitcoin/bitcoin/pull/32699#discussion_r2150261914>
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/bitcoin/bitcoin/pull/32699#iss
...

👋 Crypt-iQ's pull request is ready for review: "log: rate limiting followups"
(https://github.com/bitcoin/bitcoin/pull/33011)

💬 tnndbtc commented on issue "intermittent timeout in wallet_signer.py : 'createwallet' RPC took longer than 1200.000000 seconds":
(https://github.com/bitcoin/bitcoin/issues/32855#issuecomment-3097892555)
@naiyoma For `if(str_std_in.empty())`, it will always return true in this case. However, the empty string is sent by successful calling of `subprocess_close(err_wr_pipe);// close child side of pipe, else get stuck in read below`. If you comment out this line, you will see that read from stdin in signer.py will 100% get stuck.

🤔 l0rinc reviewed a pull request: "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline"
(https://github.com/bitcoin/bitcoin/pull/32279#pullrequestreview-3039141524)
Thanks, applied most of your comments + a few additional cleanups.
The first push was a simple rebase, the second contains the changes.

💬 l0rinc commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2219879016)
Sure, modernized the definition in a separate commit

💬 l0rinc commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2219893434)
Done

💬 l0rinc commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2219911902)
Done, thanks

💬 l0rinc commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2219916340)
Makes sense, thanks

📝 brunoerg opened a pull request: "test: check tx is final when there is no locktime"
(https://github.com/bitcoin/bitcoin/pull/33030)
According to https://corecheck.dev/mutation/src/consensus/tx_verify.cpp, there is no proper test for the `tx.nLockTime == 0` check in the `IsFinalTx` function, which is understandable, since this check will only be useful for a specific case where the `nBlockHeight` (block height) is zero. Otherwise, the following check `if ((int64_t)tx.nLockTime < ((int64_t)tx.nLockTime < LOCKTIME_THRESHOLD ? (int64_t)nBlockHeight : nBlockTime))` would catch any of it. This PR adds a test case for it.

💬 murchandamus commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2219993026)
UTXOs are ordered in descending effective value by BnB, so I don’t think the UTXO with largest value being likely to be generated first is a problem.

💬 murchandamus commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2219994826)
BnB traverses the UTXOs after sorting, so I don’t see why this would be of concern.

👍 darosior approved a pull request: "test: check tx is final when there is no locktime"
(https://github.com/bitcoin/bitcoin/pull/33030#pullrequestreview-3039322272)
Makes sense. utACK 7180b82420c0f303140a93ca635f5ac806bea77d. Could add a comment explaining this to the test as it may not be immediately obvious to a reader.

👍 l0rinc approved a pull request: "validation: ensure assumevalid is always used during reindex"
(https://github.com/bitcoin/bitcoin/pull/31615#pullrequestreview-3039244807)
ACK 01e372b25855a21d205c6ded83f6849691111d42

Thanks for fixing it - someone with more knowledge in this area should also validate it.

💬 l0rinc commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2219951005)
nit: the leading space was deliberate, it's used in other sentences as well to signal that it's a continuation.
We can either keep this style or remove it from this block comment, but only removing it on this line only is inconsistent. For simplicity and diff minimization I'd just revert this line.

💬 l0rinc commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2219955595)
nit: we don't need to drag all context into the lambda, consider (+ `()` is redundant):
```suggestion
auto activate_all_chainstate = [&chainman] {
```