💬 l0rinc commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1952842153)
to avoid possible copying we could take advantage of `emplace_back` constructing the key in-place:
```suggestion
keys.emplace_back(GenerateRandomKey());
outputs.emplace_back(COIN, GetScriptForDestination(WitnessV0KeyHash{keys.back().GetPubKey()}));
```

💬 l0rinc commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#discussion_r1952819528)
we could use `std::span` here
```suggestion
std::span<CKey> keys,
std::span<CTxOut> outputs,
```
but unfortunately that would require modernizing `CreateValidTransaction` as well - mayne in a follow-up PR :)

💬 l0rinc commented on pull request "Benchmark Chainstate::ConnectBlock duration":
(https://github.com/bitcoin/bitcoin/pull/31689#issuecomment-2654072249)
Forgot to post the diff with most of my suggestions (please double-check):

<details>
<summary>Suggestions</summary>

```patch
diff --git a/src/bench/connectblock.cpp b/src/bench/connectblock.cpp
--- a/src/bench/connectblock.cpp (revision cb071470c2d9e595b5b771b153dfe64fdd1b1392)
+++ b/src/bench/connectblock.cpp (date 1739374368802)
@@ -32,35 +32,33 @@
const WitnessV1Taproot coinbase_taproot{XOnlyPubKey(test_setup.coinbaseKey.GetPubKey())};

// Create the outputs that will
...

🤔 instagibbs reviewed a pull request: "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs"
(https://github.com/bitcoin/bitcoin/pull/31829#pullrequestreview-2612036122)
combing through tests a bit, think I spotted the CI failure cause

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1952747093)
889afbadb417e9422c7c06fd074981fa62045568

Since we're restarting does this wipe the mocktime on the node? Doesn't seem to affect timings, but I think it's easier to think about the test this way.

note that if you set it here, you also need to setmocktime any other time you restart as well

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1952818760)
this will trivially pass (and not actually ensure there was an eviction) unless you let the orphans be processed with a send_and_ping.

It's also not true, because no evictions will happen with just 1000 orphans

Here's a suggested patchset which has this subtest run in ~2m30s and actually causes evictions with default parameters: https://github.com/instagibbs/bitcoin/commit/fedea4a17b7fc4c442b0ad98b51b85ff93a55beb

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1952714733)
on second thought, this will probably throw an assertion if the final child isn't in the mempool yet? Probably need to prepend this clause with `ancestor_package[-1]["txid"] in node.getrawmempool()`

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1952893131)
this is a buggy assertion because no evictions will be happening, you might just front-run validation and slip through on your local machine. see https://github.com/instagibbs/bitcoin/commit/fedea4a17b7fc4c442b0ad98b51b85ff93a55beb for what I think would be a valid assertion

💬 pinheadmz commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2654076121)
# all.SHA256SUMS as of 096525e92cc2

```

d7308491e32e076f40e58aaa9092ceae8a16b39e66937af72bdc8b879164f304 bitcoin-096525e92cc2-aarch64-linux-gnu-debug.tar.gz
0351c42b5adf4759fd8441feb9da561d9066bbbb030d47ffa33b30eba6e9d247 bitcoin-096525e92cc2-aarch64-linux-gnu.tar.gz
9a4f902ff10ff24944a314708e59394033e72eb4bfbbacafe0bb4c74f0079be4 bitcoin-096525e92cc2-arm-linux-gnueabihf-debug.tar.gz
15f2575345370655a0e5c57ffed9f388999f9795f77ac34e99b68a86116ba721 bitcoin-096525e92cc2-arm-linux-gnue
...

👍 fanquake approved a pull request: "depends: Fix compiling `libevent` package on NetBSD"
(https://github.com/bitcoin/bitcoin/pull/31500#pullrequestreview-2612361090)
ACK f89f16846ec02942e7b81d24a85e3f40941e5426

🚀 fanquake merged a pull request: "depends: Fix compiling `libevent` package on NetBSD"
(https://github.com/bitcoin/bitcoin/pull/31500)

💬 darosior commented on pull request "fuzz: add targets for PCP and NAT-PMP port mapping requests":
(https://github.com/bitcoin/bitcoin/pull/31676#discussion_r1952906987)
Leftover from my Sock implem.

💬 furszy commented on pull request "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)":
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r1952828509)
```suggestion
// Make sure our chain tip before shutting down scores better than any other candidate
// to maintain a consistent best tip over reboots in case of a tie with another chain.
```

💬 furszy commented on pull request "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)":
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r1952920225)
q: cannot just set the tip's sequence id to 0?

We only compare the tip inside `TryAddBlockIndexCandidate` and not the entire chain (if not, then a test might be missing because setting only the tip to 0 still passes all tests).

👍 ryanofsky approved a pull request: "Doc: add a comment referencing past vulnerability next to where it was fixed"
(https://github.com/bitcoin/bitcoin/pull/30538#pullrequestreview-2612417042)
Code review ACK 7fea8eeeb9984ff6f3ed661f3970b1aaa68548de, but needs rebase currently

> I'm not sure that having a comment that references code that no longer exists is all that useful.

I think I'd agree that some of these comments are more useful than others, but some definitely seem useful, and even the ones that aren't especially useful provide context and are very interesting to know about and don't get in the way.

- d6b46d8d5e77b5fef14658340c316d4d84d95d6e seems useful and important
...

📝 0xB10C opened a pull request: "test, tracing: don't use problematic `bpf_usdt_readarg_p()`"
(https://github.com/bitcoin/bitcoin/pull/31848)
Instead of using the undocumented bcc helper `bpf_usdt_readarg_p()`, use [`bpf_usdt_readarg()`][1] and [`bpf_probe_read_user()`][2]/[`bpf_probe_read_user_str()`][3] as documented in the [bcc USDT reference guide][1].

Note that the `bpf_probe_read_user()` documentation says the following:
> For safety, all user address space memory reads must pass through bpf_probe_read_user().

It's [assumed](https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-2286505348) that using `bpf_usdt_read
...

💬 furszy commented on pull request "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)":
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r1952982015)
Nvm, answered offline by mzumsande. It's for an edge case: the user invalidates the tip after startup and before receiving any blocks from the network, and the node then finds out that there are two competing blocks for the tip's ancestor.

💬 0xB10C commented on pull request "test, tracing: don't use problematic `bpf_usdt_readarg_p()`":
(https://github.com/bitcoin/bitcoin/pull/31848#issuecomment-2654199608)
I did [>15 re-runs](https://github.com/0xB10C/bitcoin/actions/runs/12912670830 ) of https://github.com/bitcoin/bitcoin/compare/master...0xB10C:bitcoin:2024-12-dont-use-bpf_usdt_readarg_p-testing similar to the 20 runs in https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-2528671656 to make sure this won't intermittently fail again.

💬 0xB10C commented on issue "Intermittent failures in interface_usdt_mempool.py":
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-2654207216)
opened #31848 which reverts https://github.com/bitcoin/bitcoin/pull/30574 and should fix the intermittent failures.

💬 darosior commented on pull request "fuzz: add targets for PCP and NAT-PMP port mapping requests":
(https://github.com/bitcoin/bitcoin/pull/31676#discussion_r1952997408)
Good catch. Seems like this could also be useful for other harnesses.