💬 furszy commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1962092420)
Done as suggested
(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
...
(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
(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
(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
(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.
(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.
(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
...
(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
...
(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.
(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
...
(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
(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.
(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)
(https://github.com/bitcoin/bitcoin/pull/31908)
💬 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_r1962152011)
Indeed, I was briefly confused about this, but there's no need to change it.
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1962152011)
Indeed, I was briefly confused about this, but there's no need to change it.
💬 jonatack commented on pull request "Revert merge of PR #31826":
(https://github.com/bitcoin/bitcoin/pull/31908#issuecomment-2669409844)
> (Why my approve button can't click?
I believe it was disabled for non-members of the repository because we were seeing too much spamming from that functionality.
(https://github.com/bitcoin/bitcoin/pull/31908#issuecomment-2669409844)
> (Why my approve button can't click?
I believe it was disabled for non-members of the repository because we were seeing too much spamming from that functionality.
💬 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#issuecomment-2669414096)
Addressed feedback and updated the PR description.
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2669414096)
Addressed feedback and updated the PR description.
💬 theuni commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1962175261)
Probably not here. IIRC hebasto split this out of a larger commit. Agree that it's confusing, but since it's resolved later I'm not too worried. I'd say it's up to hebasto if he wants to fix it for the sake of history or not.
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1962175261)
Probably not here. IIRC hebasto split this out of a larger commit. Agree that it's confusing, but since it's resolved later I'm not too worried. I'd say it's up to hebasto if he wants to fix it for the sake of history or not.
💬 ismaelsadeeq commented on pull request "mining: drop unused -nFees and sigops from CBlockTemplate":
(https://github.com/bitcoin/bitcoin/pull/31897#issuecomment-2669440541)
> block.vtx contains a dummy coinbase, vTxFees and vTxSigOpsCost contain dummy fees and dummy sigop cost
Good point.
> That seems more straight forward to me than having vTxFees be shorter and then having to document why these things have different lengths.
We don't have to document that `vTxFees` is just a vector of fees paid in the block.
Note: This is an unblocking comment.
This PR is an immediate win because we don't have to perform checks like we do in https://github.com/bi
...
(https://github.com/bitcoin/bitcoin/pull/31897#issuecomment-2669440541)
> block.vtx contains a dummy coinbase, vTxFees and vTxSigOpsCost contain dummy fees and dummy sigop cost
Good point.
> That seems more straight forward to me than having vTxFees be shorter and then having to document why these things have different lengths.
We don't have to document that `vTxFees` is just a vector of fees paid in the block.
Note: This is an unblocking comment.
This PR is an immediate win because we don't have to perform checks like we do in https://github.com/bi
...
✅ glozow closed a pull request: "random: move VerifyRNDRRS above InitHardwareRand"
(https://github.com/bitcoin/bitcoin/pull/31902)
(https://github.com/bitcoin/bitcoin/pull/31902)