🤔 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

💬 Sjors commented on pull request "mining: add applySolution() to interface":
(https://github.com/bitcoin/bitcoin/pull/33374#discussion_r2348144204)
We could order them alphabetically, thematically or based on when they were introduced. I think the first two are better, but no strong preference.

@ryanofsky what do you think is better?

💬 Sjors commented on pull request "mining: add applySolution() to interface":
(https://github.com/bitcoin/bitcoin/pull/33374#discussion_r2348155638)
That could be a problem indeed.

Another thing that's not great here is that `m_block_template->block` is mutated. Will look into it. My assumption is that in general a client will call `submitSolution` first, since it's time sensitive, and then this method. So it should be fine if it copies to block rather than mutates the original even if that's marginally slower.

💬 Sjors commented on pull request "mining: add applySolution() to interface":
(https://github.com/bitcoin/bitcoin/pull/33374#discussion_r2348160819)
> is being run against mainnet params for ~100 blocks

It uses mainnet in order to have difficulty adjustment, but we should probably _also_ run against regtest for segwit coverage.

💬 Sjors commented on pull request "wallet: warn against accidental unsafe older() import":
(https://github.com/bitcoin/bitcoin/pull/33135#discussion_r2348170725)
Yes, it's only printed when you set the log level to debug and it's useful for debugging if the test breaks.

💬 Sjors commented on pull request "wallet: warn against accidental unsafe older() import":
(https://github.com/bitcoin/bitcoin/pull/33135#discussion_r2348172173)
There's some sort of linter command that can check this, but I forgot which.

💬 pythcoiner commented on pull request "wallet: warn against accidental unsafe older() import":
(https://github.com/bitcoin/bitcoin/pull/33135#issuecomment-3290932833)
tACK 5e9737ff49 code review + test importing few "Liana" descriptor with timelocks > 65535

💬 Sjors commented on pull request "test: Add submitblock test in interface_ipc":
(https://github.com/bitcoin/bitcoin/pull/33380#issuecomment-3291011638)
ACK 557644ee9499583b6d00efda289fb65e8359e084

Could expand it a bit:

```diff
diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py
index 1651b53bda..89c1fd7d31 100755
--- a/test/functional/interface_ipc.py
+++ b/test/functional/interface_ipc.py
@@ -186,20 +186,33 @@ class IPCInterfaceTest(BitcoinTestFramework):
balance = miniwallet.get_balance()
coinbase.vout[0].scriptPubKey = miniwallet.get_output_script()
coinbase.v
...

✅ fanquake closed an issue: "cmake: Errors not actually errors?"
(https://github.com/bitcoin/bitcoin/issues/33153)

🚀 fanquake merged a pull request: "cmake: Fix regression in `secp256k1.cmake`"
(https://github.com/bitcoin/bitcoin/pull/33379)

💬 fanquake commented on pull request "cmake: Fix regression in `secp256k1.cmake`":
(https://github.com/bitcoin/bitcoin/pull/33379#issuecomment-3291100133)
Backported to 30.x in #33356.