💬 ryanofsky commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1962059425)
In commit "rpc: handle shutdown during long poll and wait methods" (edfb4ad204e71fc266be55748290340b8791e12f)

This change seems equivalent to previous code, and previous code seems clearer. This change also makes waitforblockheight implementation less consistent with waitforblock. Would suggest:

```diff
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -406,8 +406,8 @@ static RPCHelpMan waitforblockheight()
std::optional<BlockRef> block;
if (timeout) {

...

💬 ryanofsky commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1962033871)
In commit "rpc: handle shutdown during long poll and wait methods" (edfb4ad204e71fc266be55748290340b8791e12f)

This seem inconsistent with the commit message with says "a missing tip at startup now triggers NONFATAL_UNREACHABLE instead of CHECK_NONFATAL." Is commit message just out of date?

💬 ryanofsky commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1962065125)
In commit "Have createNewBlock() wait for a tip" (a5b34064891814a998cac73c32f60d55a93e6811)

This is actually returning nullptr not nullopt

💬 ryanofsky commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1962024170)
In commit "Handle negative timeout for waitTipChanged()" (f036e5091a90b75982684c5b64db1301d3ec2515)

IMO, new handling of negative timeouts seems reasonable to keep, but mentioning them in documentation is distracting. Would be less confusing to just shorten this to "(default is forever)".

💬 achow101 commented on pull request "Revert merge of PR #31826":
(https://github.com/bitcoin/bitcoin/pull/31908#issuecomment-2669284123)
> since this code is only targeting a not officially supported platform

Ostensibly we do though, as the code is part of the aarch64 platform which we do have CI for. However, it appears that our ARM CI does not have `HWCAP2_RNG`, but it could if we switched to using AWS Graviton instances.

💬 furszy commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1962092246)
Done as suggested

💬 furszy commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1962092420)
Done as suggested

💬 furszy commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1961997545)
> This test passes in my system but one thing that's on my mind is that can this test ever become flaky? Can there be a scenario that the best block locator changes are written to the disk before the node is killed here?

That wouldn’t be a problem, it would be a fix. See #30221 and the last paragraph of #31824’s issue description.

> Do you think a time delay should be added before killing the node here?

The test will still pass if the best locator is flushed to disk before the abrupt sh
...

💬 furszy commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1962092019)
Done as suggested

💬 furszy commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1962091902)
Done as suggested

💬 furszy commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1962092648)
Done as suggested

💬 furszy commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1962084604)
> Should we stop the node again (cleanly this time) and check this on start?

The locator persistence during a clean shutdown test should already be implemented somewhere else.

💬 furszy commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1961986354)
> Bringing it back again because I feel besides the robustness IsCoinBase() brings in here

By consensus, coinbase transactions must be the first in a block. Using `IsCoinBase()` is slightly more readable but not more robust. It is also a slower check, as it verifies that all bytes in the input hash are zero for every tx in the block. I'm a ~0 here.

💬 laanwj commented on pull request "Revert merge of PR #31826":
(https://github.com/bitcoin/bitcoin/pull/31908#issuecomment-2669309907)
> Ostensibly we do though, as the code is part of the aarch64 platform which we do have CI for. However, it appears that our ARM CI does not have HWCAP2_RNG, but it could if we switched to using AWS Graviton instances.

Having the ARM64 RNG code in the first place makes sense, and we could test this on Graviton instances.

But also agree that a workaround for an errata of specific SoC series is really on the edge of what is in scope for this project. Or user code in the first place, for that
...

💬 ryanofsky commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2669343213)
From the PR description:

> The first commit allows these three methods to be used in the GUI, by removing the `IsRPCRunning()`. This simplifies the other changes.

This currently happens in the second commit, not the first commit.

I think in general PR description is a little hard to follow though. Here is how I would describe this PR:

- This PR prevents `Mining` interface methods from sometimes crashing when called during startup before a tip is connected. It also makes some RPC meth
...

👍 ryanofsky approved a pull request: "Add waitNext() to BlockTemplate interface"
(https://github.com/bitcoin/bitcoin/pull/31283#pullrequestreview-2627587014)
Code review ACK cadbd4137d84b71be26effd6a2ae177d5031345e. Main change since last review is adding back a missing `m_interrupt` check in the waitNext loop. Also made various code cleanups in both commits.

💬 achow101 commented on pull request "random: move VerifyRNDRRS above InitHardwareRand":
(https://github.com/bitcoin/bitcoin/pull/31902#issuecomment-2669380644)
Still fails on AWS Graviton:

```
/home/ec2-user/bitcoin/src/random.cpp: In function ‘bool {anonymous}::VerifyRNDRRS()’:
/home/ec2-user/bitcoin/src/random.cpp:204:13: error: ‘GetRNDRRSInternal’ was not declared in this scope
204 | if (GetRNDRRSInternal(test)) {
| ^~~~~~~~~~~~~~~~~
gmake[2]: *** [src/util/CMakeFiles/bitcoin_util.dir/build.make:440: src/util/CMakeFiles/bitcoin_util.dir/__/random.cpp.o] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:940: src/util/CM
...

💬 achow101 commented on pull request "Revert merge of PR #31826":
(https://github.com/bitcoin/bitcoin/pull/31908#issuecomment-2669383850)
ACK 3e9b12b3e0f039a8760410afed74c7e4d15afbe6

💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1962146971)
Yes, I'll update the commit message.

🚀 achow101 merged a pull request: "Revert merge of PR #31826"
(https://github.com/bitcoin/bitcoin/pull/31908)