💬 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.

💬 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).