👍 ryanofsky approved a pull request: "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods"
(https://github.com/bitcoin/bitcoin/pull/31785#pullrequestreview-2624284851)
Code review ACK 6806651d2f64d51094178cbfb23ef2f4a956aa4d
As mentioned in a comment below, I don't really like how this PR is changing startup behavior of the RPC methods without documenting it, and when it seem like it expands the scope of this PR and makes it harder to understand. Would prefer doing that in #30635 so this PR could be more focused. But current approach does seem ok too.
, and
(https://github.com/bitcoin/bitcoin/pull/31785#pullrequestreview-2624284851)
Code review ACK 6806651d2f64d51094178cbfb23ef2f4a956aa4d
As mentioned in a comment below, I don't really like how this PR is changing startup behavior of the RPC methods without documenting it, and when it seem like it expands the scope of this PR and makes it harder to understand. Would prefer doing that in #30635 so this PR could be more focused. But current approach does seem ok too.
, and
💬 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_r1960188607)
re: https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1955115023
> I made a variant of this that's a bit more in line with #30635; during startup we'll wait by using the 0 hash.
This seems ok to me, and I initially considered suggesting that, but it seemed like it expanded the scope of the PR too much and was not related to its original goals.
I think if the PR will do this, it should also include release notes that say these RPCs will now wait on startup instead of returning er
...
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1960188607)
re: https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1955115023
> I made a variant of this that's a bit more in line with #30635; during startup we'll wait by using the 0 hash.
This seems ok to me, and I initially considered suggesting that, but it seemed like it expanded the scope of the PR too much and was not related to its original goals.
I think if the PR will do this, it should also include release notes that say these RPCs will now wait on startup instead of returning er
...
💬 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_r1960264859)
In commit "rpc: clarify longpoll behavior" (6806651d2f64d51094178cbfb23ef2f4a956aa4d)
s/is not be/is not/
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1960264859)
In commit "rpc: clarify longpoll behavior" (6806651d2f64d51094178cbfb23ef2f4a956aa4d)
s/is not be/is not/
💬 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_r1960206388)
In commit "rpc: handle shutdown during long poll and wait methods" (98282f56112af33691a3fe2e1ad7c14febbb0660)
Not important but it would be nice if code style in this PR were a little more consistent. I'd prefer replacing `uint256{}` with `uint256::ZERO` and replacing `block.has_value()` with just `block` but any consistent style would be better IMO.
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1960206388)
In commit "rpc: handle shutdown during long poll and wait methods" (98282f56112af33691a3fe2e1ad7c14febbb0660)
Not important but it would be nice if code style in this PR were a little more consistent. I'd prefer replacing `uint256{}` with `uint256::ZERO` and replacing `block.has_value()` with just `block` but any consistent style would be better IMO.
💬 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_r1960150037)
re: https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1959586025
Nice catch. I agree flipping condition is a bit fragile. It also makes code less consistent and leaves "At this point TipBlock is set..." comment being not totally accurate. Would suggest a tweak to unflip and be clearer:
```diff
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -984,10 +984,11 @@ public:
notifications().m_tip_block_cv.wait(lock, [&]() EXCLUSIVE_LOCKS_REQUIRED(notificatio
...
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1960150037)
re: https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1959586025
Nice catch. I agree flipping condition is a bit fragile. It also makes code less consistent and leaves "At this point TipBlock is set..." comment being not totally accurate. Would suggest a tweak to unflip and be clearer:
```diff
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -984,10 +984,11 @@ public:
notifications().m_tip_block_cv.wait(lock, [&]() EXCLUSIVE_LOCKS_REQUIRED(notificatio
...
👍 ryanofsky approved a pull request: "rpc: add optional blockhash to waitfornewblock, unhide in help"
(https://github.com/bitcoin/bitcoin/pull/30635#pullrequestreview-2624526050)
Code review ACK 3d88cad169aa3d2d8731c3888749054be6688f1b. No changes since last review other than rebase due to conflict in a test.
(https://github.com/bitcoin/bitcoin/pull/30635#pullrequestreview-2624526050)
Code review ACK 3d88cad169aa3d2d8731c3888749054be6688f1b. No changes since last review other than rebase due to conflict in a test.
💬 achow101 commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1960280959)
It only removes sqlite as an optional dependency when the wallet is enabled, so I think having this log line is redundant. Removing the BDB line is still incorrect at this time as it can be built with and used (in this commit).
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1960280959)
It only removes sqlite as an optional dependency when the wallet is enabled, so I think having this log line is redundant. Removing the BDB line is still incorrect at this time as it can be built with and used (in this commit).
💬 achow101 commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1960282177)
Done
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1960282177)
Done
💬 achow101 commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1960282428)
Done
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1960282428)
Done
💬 achow101 commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1960282539)
Removed
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1960282539)
Removed
💬 achow101 commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1960282629)
Done
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1960282629)
Done
💬 fanquake commented on issue "cmake: (release) version handling is broken":
(https://github.com/bitcoin/bitcoin/issues/31898#issuecomment-2666533896)
I guess I'm just confused why something that is obviously a bug would be ported as-is. There's no scenario where it makes sense to have a binary output version information that doesn't match the tag it was built from.
In any case, we'll have to fix the nonsensical behaviour in master.
(https://github.com/bitcoin/bitcoin/issues/31898#issuecomment-2666533896)
I guess I'm just confused why something that is obviously a bug would be ported as-is. There's no scenario where it makes sense to have a binary output version information that doesn't match the tag it was built from.
In any case, we'll have to fix the nonsensical behaviour in master.
👍 theuni approved a pull request: "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally"
(https://github.com/bitcoin/bitcoin/pull/31662#pullrequestreview-2624538160)
utACK 9e7eb06a24b670e8e72e0233a63bbc67733d4adb
A reminder that this will require a [change in OSS-Fuzz](https://github.com/google/oss-fuzz/blob/master/projects/bitcoin-core/build.sh#L74):
`s/SANITIZER_LDFLAGS/FUZZ_LIBS/`
(https://github.com/bitcoin/bitcoin/pull/31662#pullrequestreview-2624538160)
utACK 9e7eb06a24b670e8e72e0233a63bbc67733d4adb
A reminder that this will require a [change in OSS-Fuzz](https://github.com/google/oss-fuzz/blob/master/projects/bitcoin-core/build.sh#L74):
`s/SANITIZER_LDFLAGS/FUZZ_LIBS/`
💬 hebasto commented on issue "cmake: (release) version handling is broken":
(https://github.com/bitcoin/bitcoin/issues/31898#issuecomment-2666549006)
@fanquake
Could you demonstrate how a release source tarball becomes broken when _all_ steps in the release process have been followed?
(https://github.com/bitcoin/bitcoin/issues/31898#issuecomment-2666549006)
@fanquake
Could you demonstrate how a release source tarball becomes broken when _all_ steps in the release process have been followed?
💬 TheCharlatan commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#discussion_r1960273581)
Nit: These headers in the comments seem a bit inconsistent at the moment. What makes the secp subtree special enough to get its own header? Where do these sections end? Why don't the other components get their own headers?
(https://github.com/bitcoin/bitcoin/pull/31161#discussion_r1960273581)
Nit: These headers in the comments seem a bit inconsistent at the moment. What makes the secp subtree special enough to get its own header? Where do these sections end? Why don't the other components get their own headers?
👍 TheCharlatan approved a pull request: "cmake: Set top-level target output locations"
(https://github.com/bitcoin/bitcoin/pull/31161#pullrequestreview-2624512587)
ACK f7d8a44ce42cce0b3010a7f7d14ec180e959f3e4
(https://github.com/bitcoin/bitcoin/pull/31161#pullrequestreview-2624512587)
ACK f7d8a44ce42cce0b3010a7f7d14ec180e959f3e4
💬 hodlinator commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r1960318327)
```
₿ touch A
₿ touch a
₿ ls [Aa]
a A
```
Doesn't match GNU `ls` order.. but it does match ASCII order. :+1:
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r1960318327)
```
₿ touch A
₿ touch a
₿ ls [Aa]
a A
```
Doesn't match GNU `ls` order.. but it does match ASCII order. :+1:
💬 maflcko commented on issue "cmake: (release) version handling is broken":
(https://github.com/bitcoin/bitcoin/issues/31898#issuecomment-2666613449)
For reference, I reviewed and tested this specifically for the cmake switch last year and I concluded that everything has been ported accurately. So I don't think there is a regression or a release-blocker.
In any case, it would be good to include full steps to reproduce here, starting from a fresh clone, because IIRC in some edge cases, the parent directory structure may influence the result if it is a git dir. The edge-cases aren't well documented, sometimes only mentioned in the release note
...
(https://github.com/bitcoin/bitcoin/issues/31898#issuecomment-2666613449)
For reference, I reviewed and tested this specifically for the cmake switch last year and I concluded that everything has been ported accurately. So I don't think there is a regression or a release-blocker.
In any case, it would be good to include full steps to reproduce here, starting from a fresh clone, because IIRC in some edge cases, the parent directory structure may influence the result if it is a git dir. The edge-cases aren't well documented, sometimes only mentioned in the release note
...
💬 GregTonoski commented on issue "bitcoind shouldn't fail to progress with synchronization: endless [leveldb] Generated table ... logs":
(https://github.com/bitcoin/bitcoin/issues/31882#issuecomment-2666626452)
> just saw you included logs. It's just very slow, so clearly no deadlock or livelock
I would suggest investigating the number of "generated table ..." logs. There shouldn't be so many of them especially in comparison to a correct execution.
(https://github.com/bitcoin/bitcoin/issues/31882#issuecomment-2666626452)
> just saw you included logs. It's just very slow, so clearly no deadlock or livelock
I would suggest investigating the number of "generated table ..." logs. There shouldn't be so many of them especially in comparison to a correct execution.
💬 ismaelsadeeq commented on pull request "mining: drop unused -nFees and sigops from CBlockTemplate":
(https://github.com/bitcoin/bitcoin/pull/31897#issuecomment-2666698825)
ACK d1cc874d1f87a654aa2da50dbcf850b84517e938
> I think it's more intuitive for block.vtx and these two vectors to have the same length.
No strong opinion but I tend to prefer @ryanofsky diff because it is precise. Since the coinbase transaction does not have fees, we should not simply add 0 for the size of `vTxFees` to match `vtx` .
They are two different list with different meaning
`vtx` is the list of transactions in the block, while `vTxFees` is the list of fees for each transactio
...
(https://github.com/bitcoin/bitcoin/pull/31897#issuecomment-2666698825)
ACK d1cc874d1f87a654aa2da50dbcf850b84517e938
> I think it's more intuitive for block.vtx and these two vectors to have the same length.
No strong opinion but I tend to prefer @ryanofsky diff because it is precise. Since the coinbase transaction does not have fees, we should not simply add 0 for the size of `vTxFees` to match `vtx` .
They are two different list with different meaning
`vtx` is the list of transactions in the block, while `vTxFees` is the list of fees for each transactio
...