💬 hodlinator commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2569912274)
I'm fine with including the leaf construction, but that's not my concern in this case.

`mutation`/`mutated` is used here:
https://github.com/bitcoin/bitcoin/blob/14a4dca89adaab6f1ad42af6716f9065f1abe2e1/src/validation.cpp#L3944-L3961

Which leads to this being run on every iteration through the outer `while`:
https://github.com/bitcoin/bitcoin/blob/14a4dca89adaab6f1ad42af6716f9065f1abe2e1/src/consensus/merkle.cpp#L46-L53

✅ l0rinc closed a pull request: "test: tighten `V2TransportTester::SendMessage` bounds for GCC 15.2"
(https://github.com/bitcoin/bitcoin/pull/33963)

💬 l0rinc commented on pull request "test: tighten `V2TransportTester::SendMessage` bounds for GCC 15.2":
(https://github.com/bitcoin/bitcoin/pull/33963#issuecomment-3587209651)
Thanks for checking, it seems we have to be systematic about this.

💬 maflcko commented on issue "Startup crash on macOS with GCC 15.2: `std::source_location::file_name()` is `nullptr`":
(https://github.com/bitcoin/bitcoin/issues/33964#issuecomment-3587213137)
Your toolchain clearly violates the C++ standard (https://eel.is/c++draft/support.srcloc#class.general-2), so you'll have to report this upstream via a minimal reproducer.

💬 l0rinc commented on pull request "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count":
(https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2569947721)
`ComputeMerkleRoot` is usually called without mutation checks:
https://github.com/bitcoin/bitcoin/blob/fa283d28e261416f0785450ab687af199103776a/src/validation.cpp#L3929-L3932

Most existing mutation checks are noops, see: https://github.com/bitcoin/bitcoin/pull/33805.
The benchmark was modeling pre-segwit `BlockMerkleRoot` before, and now it's closer to the behavior of `BlockWitnessMerkleRoot`.

I haven't measured it explicitly, but the latter should be more representative of modern usage - plea
...

💬 joshdoman commented on pull request "kernel: Expose reusable `PrecomputedTransactionData` in script validation":
(https://github.com/bitcoin/bitcoin/pull/33891#discussion_r2569950802)
An explicit test makes sense. Just added one.

💬 joshdoman commented on pull request "kernel: Expose reusable `PrecomputedTransactionData` in script validation":
(https://github.com/bitcoin/bitcoin/pull/33891#discussion_r2569952140)
Good suggestion, made that change.

💬 joshdoman commented on pull request "kernel: Expose reusable `PrecomputedTransactionData` in script validation":
(https://github.com/bitcoin/bitcoin/pull/33891#discussion_r2569953795)
No problem.

💬 joshdoman commented on pull request "kernel: Expose reusable `PrecomputedTransactionData` in script validation":
(https://github.com/bitcoin/bitcoin/pull/33891#discussion_r2569960162)
To try to mitigate this concern, I renamed the parameter `precomputed_txdata`, to make it more clear that it pertains to the actual transaction being verified (previously the parameter was more vaguely named `precomputed_data`).

I also updated the parameter description in the documentation for `btck_script_pubkey_verify`, to make it explicit that the parameter is derived from `tx_to` and the spent outputs.

Hopefully these changes help, but open to other suggestions!

💬 joshdoman commented on pull request "kernel: Expose reusable `PrecomputedTransactionData` in script validation":
(https://github.com/bitcoin/bitcoin/pull/33891#discussion_r2569960497)
Done!

💬 hodlinator commented on pull request "ci: Make the max number of commits tested explicit":
(https://github.com/bitcoin/bitcoin/pull/33909#discussion_r2569972760)
Explored that now, but we still have the ci-test-each-commit-exec.py script...

https://github.com/bitcoin/bitcoin/blob/d14d1b52b51c4e8e2388af7e54c854e8a133e088/.github/workflows/ci.yml#L113

...and unlike the `name`, this job "id" is not externally facing, so I'd rather not touch it.

💬 hodlinator commented on pull request "ci: Make the max number of commits tested explicit":
(https://github.com/bitcoin/bitcoin/pull/33909#discussion_r2569986901)
I've spent more time than I care to admit together with an underpowered LLM and StackOverflow to come up with ways to avoid duplicating the number 6 in both the job's `name` and the `MAX_COUNT` constant.

Haven't found a way to access `env.*` when setting `name`, or to access `name` when setting `env.*` (`github.job` returns "test-each-commit"). I've also explored doing it through YAML node aliases, but that doesn't seem to work either.

I want to specify the 6 in the name in order to make i
...

💬 l0rinc commented on pull request "ci: Make the max number of commits tested explicit":
(https://github.com/bitcoin/bitcoin/pull/33909#discussion_r2570013937)
Why is it better to duplicate than to abstract the above to what we should pay attention to? It's not the 6 that matters, but that not all of them, so why not focus on that instead?

💬 hodlinator commented on pull request "ci: Make the max number of commits tested explicit":
(https://github.com/bitcoin/bitcoin/pull/33909#discussion_r2570018072)
Hm.. I guess a shorter version of "test a limited number of ancestor commits" could work.
"test a few ancestor commits"?

💬 l0rinc commented on pull request "ci: Make the max number of commits tested explicit":
(https://github.com/bitcoin/bitcoin/pull/33909#discussion_r2570025965)
It should signal that it's not exhaustive, something slightly ambiguous like "test latest commits" or "test latest few commits"?

💬 151henry151 commented on pull request "Align legacy script policy with P2SH policy in AreInputsStandard":
(https://github.com/bitcoin/bitcoin/pull/33926#issuecomment-3587403889)
> Please note that contributors are required to fully understand their authored code themselves.


The core change applies sigop limits to legacy scripts and fixes CLEANSTACK to only apply to P2SH as specified in BIP62. Most of the diff is test updates—the functional tests were using 0-sigop scripts that fail under the new policy, so they needed to be switched to 1-sigop scripts with proper signing.

I've been stuck on a `p2p_segwit.py` failure for quite a while now. The issue appears to be
...

💬 151henry151 commented on issue "node0 stderr bitcoind: validation.cpp:5392: void ChainstateManager::CheckBlockIndex() const: Assertion `!c->setBlockIndexCandidates.contains(const_cast<CBlockIndex*>(pindex))' failed.":
(https://github.com/bitcoin/bitcoin/issues/33948#issuecomment-3587488953)
This looks similar to #32173 (same line in CheckBlockIndex). Could the experimental LLVM 22 compiler be exposing a logic bug in setBlockIndexCandidates maintenance, similar to issues fixed in #16849 and #30479? The compiler might be hitting a consistency issue with setBlockIndexCandidates that happens when cs_main is released during block processing.

💬 fjahr commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#issuecomment-3587495871)
Had another thought: since there are quite a bit of small-ish helper functions refactored or replaced with different ones, it might be interesting to check a coverage report if there are any of these new functions that aren't covered and that coverage didn't go down somewhere in general. I didn't have time to look into this myself yet though.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2570548607)
They are sorted now and not sorted in the snippet you pasted above, e.g.:

```
ADDRMAN_ADDRESSES = [
"5.2.154.6",
"1.65.195.98",
"18.27.79.17",
"2.59.236.56",
```
I must be missing something...

💬 maflcko commented on pull request "ci: Make the max number of commits tested explicit":
(https://github.com/bitcoin/bitcoin/pull/33909#issuecomment-3588184960)
lgtm ACK b5a7a685bba312a780eddcb4a53ce2c26a937854