💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-2653927636)
> Agree. Strings are sensitive to typos, and require extra effort for matching when doing statistics. i think even the current enumeration is better than that. BIP155 is a good suggestion, but as you have to make up non-standard values, and handle NET_UNROUTABLE differently i'm not entirely sure.
> The purpose of tracing is to gain insight into internal state so logging the actual values makes more sense than thinking up a new convention. The internal enumeration is likely stable enough to warr
...

💬 darosior commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#issuecomment-2653935678)
I'll have a look at whether it's worth storing flags representing what rules have been checked against a block in the index, to enable re-validating past connected blocks against new rules when upgrading. In the meantime, this can be closed as i do not think it is worth doing on its own.

✅ darosior closed a pull request: "Double check all block rules in `ConnectBlock`, not only `CheckBlock`"
(https://github.com/bitcoin/bitcoin/pull/31792)

🤔 ryanofsky reviewed a pull request: "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods"
(https://github.com/bitcoin/bitcoin/pull/31785#pullrequestreview-2612158513)
Code review be15f2e769987b1df2cf1c8f327c5aafe064dbb3. Mostly looks good but I think vasild's suggestion https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1952318566 should be used so waitTipChanged can't return an inconsistent hash and height

💬 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_r1952794396)
re: https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1952303154

I don't think suggested text is accurate because if timeout is exceeded while waiting for a new tip, this returns information about the current tip. I don't think that behavior should be changed.

Returning null **only** when node is shutting down should be the simplest and best behavior.

💬 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]};
```