💬 l0rinc commented on issue "bench: unrealistic ConnectBlock benchmarks":
(https://github.com/bitcoin/bitcoin/issues/33375#issuecomment-3290237829)
@Raimo33 if you can provide an alternative that's reasonably simple for a micro-benchmark, I will happily review it.

But note that I already have a few micro-benchmark improvements in that area where review is needed:
* https://github.com/bitcoin/bitcoin/pull/32554 - creates a configurable block so that we can measure the composition of the block
* https://github.com/bitcoin/bitcoin/pull/32729/files#diff-547fa26a77a99ccd77aca7ce1c69c0544666f788d463dc7bff664001f9ff1c88R40 - splits checkblock mea
...

💬 l0rinc commented on pull request "common: Make arith_uint256 trivially copyable":
(https://github.com/bitcoin/bitcoin/pull/33332#issuecomment-3290241610)
+1 for https://github.com/davidgumberg/bitcoin/commit/eda422cfa56245a9b78b9a4f499a490d44e58eb5 but without the explicit stuff, it's just noise, let the compiler do the work. We can add tests or static assertions instead of making simple structures more complicated - there's a reason the compiler is willing to generate it for us.

💬 l0rinc commented on pull request "p2p: Correct unrealistic headerssync unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#issuecomment-3290253231)
reACK cc5dda1de333cf7aa10e2237ee2c9221f705dbd9

The only difference after the rebase is a removal of a `constexpr` in one of the `base_uint` constructors.

> changed PR-title (category) from "headerssync:" to "p2p

nit: the commits still state `headerssync` (fine by me, just noticed)

💬 hMsats commented on issue "v30.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3290439495)
Not sure if this is useful but anyway: Compiled bitcoin-30.0rc1 (it's called v30.0.0rc1 in debug.log) with cmake on Ubuntu 24.04.3 LTS on an Acer Aspire E1-572, 64 bits laptop with a 4 TB external ssd. All tests (test_bitcoin, test_bitcoin-qt) passed for both bitcoind and bitcoin-qt and bitcoin-qt looks fine. Upgraded bitcoind from version 29.0 to 30.0rc1. Bitcoind (30.0rc1) is working great (txindex=1) with low block verification times (about 1 second for "Saw new cmpctblock header" to "Update
...

💬 w0xlt commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#issuecomment-3290444826)
Approach ACK.

> beneficial to extend this PR to show the reason for why script verification was enabled.

I agree.

> split the above suggestion into small focused commits

Commits could be squashed but but keeping them separate makes it clearer to review.

💬 w0xlt commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2347899862)
Just to make clearer what `60 * 60 * 24 * 7 * 2` means.

```cpp
constexpr int64_t TWO_WEEKS = 14 * 24 * 60 * 60;
if (GetBlockProofEquivalentTime(... ) <= TWO_WEEKS) { ... }
```

Or (not tested)

```cpp
#include <chrono>
using namespace std::chrono;

constexpr auto TWO_WEEKS = 14d; // 14 days

if (GetBlockProofEquivalentTime(...) <= duration_cast<seconds>(TWO_WEEKS).count()) {
...
}
```

💬 l0rinc commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2347916838)
Seems a bit risky this close to the release, maybe we can do that in a follow-up

💬 w0xlt commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2347918277)
Provide safe fallback in release builds. Aborting solely because of logging seems unnecessary.

```suggestion
default: return "unknown reason";
```

💬 w0xlt commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2347923281)
nit:
```suggestion
case CHECKED_HASH_NOT_IN_HEADERS: return "assumevalid hash not found in headers";
```

💬 w0xlt commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2347925235)
nit: logs shouldn’t expose internal variable names

```suggestion
case CHECKED_BELOW_MIN_CHAINWORK: return "best header chainwork below -minimumchainwork";
```

🤔 w0xlt reviewed a pull request: "log: always print initial signature verification state"
(https://github.com/bitcoin/bitcoin/pull/33336#pullrequestreview-3222992176)
ACK https://github.com/bitcoin/bitcoin/pull/33336/commits/63dc937eea297332ec85c1abbcf3b94b8f74aa85 with non-blocking nits mentioned above.

💬 l0rinc commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2347935159)
This was deliberate, are you suggesting that's not grammatically correct?
"File not found" -> we don't usually say "File *is* not found". The internets tell me this is called "elliptical sentence" with "implied copula".

💬 l0rinc commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2347936278)
I'm curious what others think. The current one makes the error searchable, that's why I left it as such.

💬 l0rinc commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2347937789)
I want a very obvious error if this isn't correct - I don't like these soft failures. What do other reviewers think?

💬 w0xlt commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2347984680)
Can this multiplication overflow `size_t` ?
What happens if someone passes a negative value to `-dbcache`?

Maybe worth pulling the below snippet into a helper so we can reuse it here.

https://github.com/bitcoin/bitcoin/blob/d20f10affba83601f1855bc87d0f47e9dfd5caae/src/node/caches.cpp#L31-L34

💬 ajtowns commented on pull request "mining: log failed blocks in submitSolution()":
(https://github.com/bitcoin/bitcoin/pull/33372#issuecomment-3290702101)
I think logging failed blocks would run afoul of rate limiting (#32604) unless it were special-cased similarly to the UpdateTip log message.

🤔 ajtowns reviewed a pull request: "mining: add applySolution() to interface"
(https://github.com/bitcoin/bitcoin/pull/33374#pullrequestreview-3223123539)
I like this better than just logging stuff, for whatever that's worth.

💬 ajtowns commented on pull request "mining: add applySolution() to interface":
(https://github.com/bitcoin/bitcoin/pull/33374#discussion_r2348023413)
The existing entries seem to be in order of the `@n` value rather than alphabetically.

💬 ajtowns commented on pull request "mining: add applySolution() to interface":
(https://github.com/bitcoin/bitcoin/pull/33374#discussion_r2348048893)
I think this assumes that the provided coinbase tx includes the coinbase witness, which is different to how `submitblock` behaves. (In particular `submitblock` includes a call to `chainman.UpdateUncommittedBlockStructures`, whereas `AddMerkleRootAndCoinbase` doesn't) That seems like it might be a source of bugs -- might at least be worth adding a check and having an explicit log message if the coinbase requires a witness but doesn't have it?

FWIW I think `miner_tests` is being run against mai
...

💬 ajtowns commented on pull request "RFC: blocks: add `-reobfuscate-blocks` arg to xor existing blk/rev on startup":
(https://github.com/bitcoin/bitcoin/pull/33324#issuecomment-3290838371)
tidy wants emplace_back over push_back