🤔 ryanofsky reviewed a pull request: "wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs"
(https://github.com/bitcoin/bitcoin/pull/28868#pullrequestreview-1854957206)
Code review ACK b093e5ea29efa44626db8aeff05ad2ee3296e707, but only lightly reviewed the tests.

This PR has a merge conflict currently so needs to be updated. Also I think the place where reload_wallet lamba is moved is a little unsafe so would suggest changing that (see below).

💬 ryanofsky commented on pull request "wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs":
(https://github.com/bitcoin/bitcoin/pull/28868#discussion_r1473555525)
In commit "wallet: Reload the wallet if migration exited early" (d8002ed333e13e54726bcd922303cb1e42544b44)

I was confused by "As these checks do not change anything in the wallet, such unloaded wallets should be reloaded prior to exiting" in the commit description, since the wallets would need to be reloaded whether or not checks changed anything.

I think the commit message can just say "Such unloaded wallets should be reloaded prior to exiting" without the "As these checks" part

💬 ryanofsky commented on pull request "wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs":
(https://github.com/bitcoin/bitcoin/pull/28868#discussion_r1473532556)
In commit "wallet: Reload the wallet if migration exited early" (d8002ed333e13e54726bcd922303cb1e42544b44)

I don't think it is good to move `reload_wallet` here before `BackupWallet` is called. It seems like it would make it easy to add an innocent call to `reload_wallet(local_wallet)` that looks like it is just reloading the wallet, but doesn't return right away, so the wallet get added to `wallet_dirs` before it is backed up and possibly deleted as part of the `fs::remove_all` loop.

Most
...

💬 amitiuttarwar commented on pull request "addrman: delete addresses that don't belong to the supported networks":
(https://github.com/bitcoin/bitcoin/pull/29330#issuecomment-1920125508)
+1 to the questions asked by @naumenkogs. I agree with the statement in the OP that this change would "avoid a lot of unnecessary addrman `Select` calls (especially when...", but would like additional context as to how that would tangibly impact the node operations or end user.

in general I am hesitant to add init params since it increases the combinatoric space of startup possibilities & they are hard to deprecate over time. maybe one idea (if we want to support this feature) could be to ad
...

💬 1440000bytes commented on pull request "addrman: delete addresses that don't belong to the supported networks":
(https://github.com/bitcoin/bitcoin/pull/29330#issuecomment-1920304941)
> This PR adds a new flag -cleanupaddrman. When initializing a node with this flag, it will delete addresses from addrman (both new and tried tables) that do not belong to the supported networks (e.g. -onlynet).

I don't think this is a good idea

> This flag is not enabled by default.

Concept ACK because it's not enabled by default and could be useful for testing.

💬 tdb3 commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1473699881)

Pulled changes (771d1e1d206efe687b8661ab966cc1a62cc7ba39), built, ran all functional tests (all passed). Re-ran the following on 771d1e1d206efe687b8661ab966cc1a62cc7ba39 and on release v26.0.


## Action (1): __Unexpected difference__
`curl --user test --data-binary '[]' -H 'content-type: text/plain;' http://127.0.0.1:38332/`

Result: Unexpected difference

v26.0 behavior: `[]`

PR 27101 (771d1e1d206efe687b8661ab966cc1a62cc7ba39) behavior: (no content) (in alignment with 2.0 spec, b
...

💬 tdb3 commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1473700874)
## Action (5): __Successfully executed response, legacy behavior retained__
`curl --user test --data-binary '{"jsonrpc":"1.0","method":"getblockchaininfo","id":"tester"}' -H 'content-type: text/plain;' http://127.0.0.1:38332/`

Result: Successfully executed response

v26.0 behavior: `{"result":{"chain":"signet","blocks":4320,"headers":4320,"bestblockhash":"0000013e07d388dd5c8cb66c990caa546fc04e7533cad18c27ed3d603f37a6c0","difficulty":0.001127075738664131,"time":1706750596,"mediantime":17067
...

⚠️ starius opened an issue: "build warnings in outputtype.cpp: may be used uninitialized"
(https://github.com/bitcoin/bitcoin/issues/29359)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

Building Bitcoin Core from source using official instructions from https://github.com/bitcoin/bitcoin/blob/master/doc/build-unix.md I got compilation warnings, complaining about may be used uninitialized variables in two destructors.

Log of `make -j 10`:
[bitcoin-compilation-warning-master.txt](https://github.com/bitcoin/bitcoin/files/14119565/bitcoin-compilation-warning-master.txt)


...

💬 hernanmarino commented on pull request "Modify command line help to show support for BIP21 URIs":
(https://github.com/bitcoin-core/gui/pull/752#discussion_r1473761422)
Done

💬 hernanmarino commented on pull request "Modify command line help to show support for BIP21 URIs":
(https://github.com/bitcoin-core/gui/pull/752#discussion_r1473761491)
Done

💬 hernanmarino commented on pull request "Modify command line help to show support for BIP21 URIs":
(https://github.com/bitcoin-core/gui/pull/752#issuecomment-1920456532)
Fixed the 2 typos mentioned by @luke-jr and @kristapsk . Also rebased for CI.

💬 hernanmarino commented on pull request "Modify command line help to show support for BIP21 URIs":
(https://github.com/bitcoin-core/gui/pull/752#issuecomment-1920461069)
Fixed the 2 typos mentioned by @luke-jr and @kristapsk . Also rebased for CI.

💬 stratospher commented on pull request "test: use v2 everywhere for P2PConnection if --v2transport is enabled":
(https://github.com/bitcoin/bitcoin/pull/29358#issuecomment-1920563086)
> Hmm, "test each commit fails" The problem is that commit https://github.com/bitcoin/bitcoin/commit/606f4f32014f029a6999d7f94b7231fefafdf55f (which switches P2PConnection to v2 by default) is enabled last. It might be best to squash everything into one big commit (even if that might make review a bit harder) or I'd have to add some temporary workarounds. Opinions?

would reordering the commits to keep the last commit as first commit work? reviewing is definitely easier with separate commits.

💬 maflcko commented on issue "build warnings in outputtype.cpp: may be used uninitialized":
(https://github.com/bitcoin/bitcoin/issues/29359#issuecomment-1920779707)
I guess regardless of the g++ version used on Ubuntu or Debian, some kind of false positive warnings will be printed. I wonder why g++ on other distros does not print those warnings.

👍 MarnixCroes approved a pull request: "Modify command line help to show support for BIP21 URIs"
(https://github.com/bitcoin-core/gui/pull/752#pullrequestreview-1855939371)
ack ede5014c445dcb40ddcfdede2c51236bbfe85f5e

💬 maflcko commented on pull request "log: Nuke error(...)":
(https://github.com/bitcoin/bitcoin/pull/29236#issuecomment-1920925835)
rebased

⚠️ maflcko opened an issue: "ci: Android NDK has too old libc++"
(https://github.com/bitcoin/bitcoin/issues/29360)
The android CI task fails because the embedded libc++ in the NDK is too old.

Can it be bumped?

💬 maflcko commented on issue "ci: Android NDK has too old libc++":
(https://github.com/bitcoin/bitcoin/issues/29360#issuecomment-1920972420)
Reference:

```
./util/fs.h:65:30: error: no matching constructor for initialization of 'const std::u8string &' (aka 'const basic_string<char8_t> &')
const std::u8string& utf8_str{std::filesystem::path::u8string()};
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/ci_container_base/depends/SDKs/android/ndk/23.2.8568313/toolchains/llvm/prebuilt/linux-x86_64/bin/../sysroot/usr/include/c++/v1/string:794:40: note: candidate constructor not viable: no known conv
...

👍 kristapsk approved a pull request: "Modify command line help to show support for BIP21 URIs"
(https://github.com/bitcoin-core/gui/pull/752#pullrequestreview-1856201976)
utACK ede5014c445dcb40ddcfdede2c51236bbfe85f5e

⚠️ Xaspr reopened an issue: "Unable to sync blockchain on laptop: ERROR: ReadBlockFromDisk: Deserialize or I/O error"
(https://github.com/bitcoin/bitcoin/issues/29255)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

After installing Bitcoin Core 26.0 in Windows Pro 11 and starting IBD, Core crashes.

I know Windows might not be exactly ideal, but I like to run a pruned node on my work laptop as well. I didn't set pruning yet by the way, I started Core just with the standard settings.

Bitcoin Core crashes and the following error pops up in debug.log:

`ERROR: ReadBlockFromDisk: Deserialize or
...