💬 ryanofsky 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_r1952788322)
In commit "rpc: handle shutdown during long poll and wait methods" (ad3af401c19b8d05ce69011a359db3090b7018e1)

Nice suggestion. If this suggestion is applied should also simplify the code above

```diff
- if (!tip_hash || chainman().m_interrupt) return {};
+ if (chainman().m_interrupt) return {};
```

since `tip_hash` is also relevant. Can also reduce the scope of the `tip_hash` variable.

💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1952804176)
This should have been
```patch
- assert_equal(0, closed_connection.conn.network)
+ assert_equal(NETWORK_TYPE_UNROUTABLE, closed_connection.conn.network)
```

🚀 fanquake merged a pull request: "ci: Skip read-write of default env vars"
(https://github.com/bitcoin/bitcoin/pull/31678)

🚀 fanquake merged a pull request: "build: simplify by flattening the dependency graph"
(https://github.com/bitcoin/bitcoin/pull/30911)

💬 maflcko commented on pull request "ci: limit max stack size to 512 KiB":
(https://github.com/bitcoin/bitcoin/pull/31367#issuecomment-2653979844)
> It looks like `DEBUG=1` has an effect on this?

Maybe just enable it for the two GHA macOS tasks, which do not have DEBUG=1 enabled and also do not have sanitizers enabled?

👍 ryanofsky approved a pull request: "logging: Ensure -debug=0/none behaves consistently with -nodebug"
(https://github.com/bitcoin/bitcoin/pull/31767#pullrequestreview-2612274487)
Code review ACK 7afeaa24693039730c9ae00c325c2b9d0e2deda1. Nicely implemented change with test and release notes, and I like how the test is implemented as the first commit.

✅ s373nZ closed a pull request: "[WIP] build: name install targets as components"
(https://github.com/bitcoin/bitcoin/pull/31847)

💬 s373nZ commented on pull request "[WIP] build: name install targets as components":
(https://github.com/bitcoin/bitcoin/pull/31847#issuecomment-2654017607)
Closing in favor of #31844.

💬 hebasto commented on pull request "depends: Use `CC_FOR_BUILD` for `config.guess `":
(https://github.com/bitcoin/bitcoin/pull/29963#issuecomment-2654027738)
My Guix build:
```
aarch64
ab623f96d21b9651618ccd3dd1dd547494ecbe199cf1146dfebde4d02cd141a4 guix-build-38d13e68e276/output/aarch64-linux-gnu/SHA256SUMS.part
1f70c006d53d319ebbfb915a909419d5084489e527c8cf24369f113c96705251 guix-build-38d13e68e276/output/aarch64-linux-gnu/bitcoin-38d13e68e276-aarch64-linux-gnu-debug.tar.gz
ae9eb92cdd2eee116bd48199ee9d8093082f58ce158badd47aaaa3cd044d2d9a guix-build-38d13e68e276/output/aarch64-linux-gnu/bitcoin-38d13e68e276-aarch64-linux-gnu.tar.gz
31c6976e
...

💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1952873395)
Ok, I see your arguments. Feel free to ignore this comment of mine and leave the code as it is. I do not see it as a blocker.

Yes, I assumed that `try_exec()` will never return `true` and I guessed that `try_exec()` does an exec syscall without reading its code. Both Windows and Linux docs are clear that `execvp()` only returns if it couldn't execute the program. `try_exec()` could be:

```diff
auto try_exec = [&](fs::path exe_path, bool allow_notfound = true) {
std::string
...

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2654044188)
Thank you for all the suggestions made over the past week @stickies-v!

Updated 817865d57daa822370b0f67e1e079fdd25ab3130 -> efbd187a1ff4409ca54ef94bcd97359e0948077e ([kernelApi_22](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_22) -> [kernelApi_23](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_23), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_22..kernelApi_23))

* Adapted @stickies-v's suggestion made in https://github.com/TheCharlatan/bitcoin/pull/24
...

💬 s373nZ commented on pull request "cmake: add a component for each binary":
(https://github.com/bitcoin/bitcoin/pull/31844#issuecomment-2654053598)
ACK fb0546b1c5ebb858605bef4c9fa001782e0ab213

Tested lightly by building and installing for targeted components `bitcoind` and `bitcoin-cli` as described in the PR description.

Should be mentioned this also closes #31745.

💬 l0rinc commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1952684824)
```suggestion
// Doing this in a separate block excludes the validation of its inputs from the benchmark
```

----


But more importantly, why are we doing this in the first place? Why do we want to exclude the validation of the inputs? Is that an important usecase?

💬 l0rinc commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1952729267)
It looks to me like we could do this more consistently:
```suggestion
auto& chainman{test_setup.m_node.chainman};
auto& chainstate{chainman->ActiveChainstate()};
{
LOCK(::cs_main);
auto* pindex{chainman->m_blockman.AddToBlockIndex(test_block, chainman->m_best_header)};
chainman->ActiveChain().SetTip(*pindex);
}
...
assert(chainstate.ConnectBlock(test_block, test_block_state, chainman->ActiveChain().Tip(), viewNew));
```
but even if
...

💬 l0rinc commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1952803612)
we can avoid copying this:
```suggestion
const auto& coinbase_to_spend{test_setup.m_coinbase_txns[0]};
```

💬 l0rinc commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1952761514)
I don't like this separation of first and rest - it introduces state in this loop that's really hard to follow.

I don't yet understand this part (we do we need a separate `CreateValidTransaction` call for the first tx here, I don't get why that's special - since it's not a coinbase, right?), but if we have to keep it, consider simplifying to something like this inside the loop:
```C++
const auto& tx_to_spend{i == 0 ? first_tx : txs.back()};
```

💬 l0rinc commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1952694171)
```suggestion
* the different signature algorithms
```

💬 l0rinc commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1952719974)
nit: we don't necessarily need the `std::vector` constructs here, we could reduce the nose slightly (please see other uses of it here):
```suggestion
const auto [first_tx, _]{test_setup.CreateValidTransaction(
{coinbase_to_spend},
{COutPoint(coinbase_to_spend->GetHash(), 0)},
chainstate.m_chain.Height() + 1, keys, outputs, {}, {})};
const auto test_block_parent_coinbase{GetScriptForDestination(coinbase_taproot)};
test_setup.CreateAndProcessBlock({first_
...

💬 l0rinc commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1952857029)
These names are global, it may not be obvious what "ConnectBlockMixed" means outside of this context:
```suggestion
BENCHMARK(ConnectBlockMixedEcdsaSchnorr, benchmark::PriorityLevel::HIGH);
```

💬 l0rinc commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1952800973)
I've checked the price of calling this in every iteration:
1,000,387.85 blocks / s without `ConnectBlock` (measuring `CCoinsViewCache` creation only) versus 28.06 blocks / s for both - so 👍 we're mostly measuring `ConnectBlock` speed here.