💬 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] {
```

💬 l0rinc commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2219977333)
could we do this change in a separate commit, explaining in the commit messages separately why this needed?
And I might be wrong here but I don't understand why we need to run this twice - which is the case for reindex as far as I can tell.

💬 l0rinc commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2219981258)
does this comment still make sense?

💬 l0rinc commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2220008096)
pretty good test, it covers every new added line!

💬 l0rinc commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2220011447)
What's the reason for moving this? If it's needed, could you add it to the commit message?

💬 murchandamus commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2220021453)
I think we are crossing wires here. I was talking about running coin selection with the **same UTXO set** at two different feerates. You appear to be talking about running a test with two different UTXO sets generated from the same set of _effective values_ at different feerates.

💬 murchandamus commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2220043055)
The test you propose is also interesting, and someone might want to take a stab at adding it in another pull request. It is out of scope for this pull request however.

Fuzzing explores pseudorandomly all code paths that can be reached by exercising the fuzz harness. By comparing BnB with a brute force search, we can demonstrate that BnB always finds the best solution that is present, but only when they are present. As only inputs that cause new paths to be explored in the source code are reta
...

💬 murchandamus commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#issuecomment-3098071484)
Removed the duplicate definition of `coin_params.m_cost_of_change`.

💬 mzumsande commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2219983800)
I wonder if it wouldn't be simpler if all of the `m_block_xyz` class members were just local variables within `CoinStatsIndex::CustomAppend` instead.
The previous values are being overwritten here with each new block, and as far as I can see the saved values (while being set to correct values in `CustomInit` and `ReverseBlock`) are not used anywhere - except for
[this line](https://github.com/fjahr/bitcoin/blob/4cb2c0520211634ee3caf6d4c3a1e65b2bdb2dc6/src/index/coinstatsindex.cpp#L483C5-L483C
...

👍 ryanofsky approved a pull request: "rpc: Handle -named argument parsing where '=' character is used"
(https://github.com/bitcoin/bitcoin/pull/32821#pullrequestreview-3039323143)
Code review ACK 56b1da77b5998c44b3bb799689a6c0449e9223fc. Thanks for the update! The fix here seems right and should be practically useful. And the new tests seem to cover different scenarios where positional string parameters contain '='.

Two comments:

- I think it would good to add a test to cover cases where positional *non-string* parameters contain '=', i.e. the `bitcoin-cli -named echojson '["fail=yes"]'` case which [previously failed](https://github.com/bitcoin/bitcoin/pull/32821#di
...

💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2220035865)
In commit "test: Add functional tests for named argument parsing" (56b1da77b5998c44b3bb799689a6c0449e9223fc)

Note: this comment is not exactly describing what is happening during this test. It's not true in general that unknown parameters with `=` are treated as positional. Unknown parameters with `=` are only treated as positional if the next positional parameter is known and either accepts a string (this case) or if it's known and accepts JSON and the argument actually parses as json. Other
...

💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2220009660)
re: https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2203147913

Thanks! it appears all issues are solved now with the latest push (56b1da77b5998c44b3bb799689a6c0449e9223fc)