💬 achow101 commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1960261844)
No, watch-only descriptor wallets do not change the GUI as watch-only legacy wallets did.

💬 theuni commented on pull request "cmake: Add optional sources to `minisketch` library directly":
(https://github.com/bitcoin/bitcoin/pull/31880#issuecomment-2666500965)
> > What's the advantage here over simply making `minisketch_clmul` static?
>
> To use the same approach across all similar cases in the project.

Yeah, ok, agreed.

💬 hebasto commented on issue "cmake: (release) version handling is broken":
(https://github.com/bitcoin/bitcoin/issues/31898#issuecomment-2666502305)
> What was the autotools behavior, to compare?

I've reproduced all steps from https://github.com/bitcoin/bitcoin/issues/31898#issue-2860887337 for the pre-CMake [commit](https://github.com/bitcoin/bitcoin/commit/80f00cafdeef0600fa1a5e906686720786c2336c):
```
$ tar xf bitcoin-99.99.tar.gz
$ cd bitcoin-99.99/
$ ./autogen.sh
$ ./configure
$ make -j $(nproc)
$ ./src/bitcoind --version
Bitcoin Core version v28.99.0-g80f00cafdeef0600fa1a5e906686720786c2336c
Copyright (C) 2009-2024 The Bitcoin Core
...

👍 theuni approved a pull request: "cmake: Add optional sources to `minisketch` library directly"
(https://github.com/bitcoin/bitcoin/pull/31880#pullrequestreview-2624493666)
utACK 9919e92022ba61d2dc1b13b1116b56035be459a6

One less autotools-ism and CMake hack.

💬 fanquake commented on issue "cmake: (release) version handling is broken":
(https://github.com/bitcoin/bitcoin/issues/31898#issuecomment-2666507401)
So then the bug was just ported/these code paths untested during the transition?

💬 achow101 commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1960266934)
`importaddress` calls `importdescriptors` in the test framework. These checks are that `importaddress` (the legacy wallet RPC) doesn't rescan and will require a separate rescan. But that will not work once the tests is descriptor wallets only.

Will expand the commit message.

💬 hebasto commented on issue "cmake: (release) version handling is broken":
(https://github.com/bitcoin/bitcoin/issues/31898#issuecomment-2666510689)
> So then the bug was just ported/these code paths untested during the transition?

The mirroring of behaviour was tested.

👍 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

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

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

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

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

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

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

💬 achow101 commented on pull request "wallet: Disable creating and loading legacy wallets":
(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

💬 achow101 commented on pull request "wallet: Disable creating and loading legacy wallets":
(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

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

👍 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/`