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

💬 Eunovo commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2348327967)
I think it still makes sense to account for this scenario since minimumchainwork and assumevalid block are set separately. I'll update the comment to explain that minimumchainwork only needs to be skipped if you set an older assumevalid block without also reducing the minimumchainwork.

📝 vasild opened a pull request: "test: don't throw from the destructor of DebugLogHelper"
(https://github.com/bitcoin/bitcoin/pull/33388)
Throwing an exception from the destructor of a class is a bad practice because the destructor will be called when an object of that type is alive on the stack and another exception is thrown, which will result in "exception during the exception". This would terminate the program without any messages.

Instead print the message to the standard error output and call `std::abort()`.

---

This change is part of https://github.com/bitcoin/bitcoin/pull/26812. It is an improvement on its own, so
...

💬 vasild commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2348333882)
Done in https://github.com/bitcoin/bitcoin/pull/33388