💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1919923411)
Rebased cb47c334e0b3d654b5b56ae762aa2a2c33ae2743 -> 6752d218caeed1111f2520130c156b9ef42ba805 ([noGlobalSignals_12](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_12) -> [noGlobalSignals_13](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_13), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_12..noGlobalSignals_13))

* Fixed silent merge conflict with https://github.com/bitcoin/bitcoin/pull/28170

💬 achow101 commented on pull request "Nuke adjusted time from validation (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#issuecomment-1919931046)
ACK ff9039f6ea876bab2c40a06a93e0dd087f445fa2

🚀 achow101 merged a pull request: "Nuke adjusted time from validation (attempt 2)"
(https://github.com/bitcoin/bitcoin/pull/28956)

🤔 TheCharlatan reviewed a pull request: "Nuke adjusted time from validation (attempt 2)"
(https://github.com/bitcoin/bitcoin/pull/28956#pullrequestreview-1854827936)
ACK ff9039f6ea876bab2c40a06a93e0dd087f445fa2

💬 mzumsande commented on pull request "test: use v2 everywhere for P2PConnection if --v2transport is enabled":
(https://github.com/bitcoin/bitcoin/pull/29358#issuecomment-1919948720)
Hmm, "test each commit fails" The problem is that 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?

🚀 ryanofsky merged a pull request: "wallet: Fix migration of blank wallets"
(https://github.com/bitcoin/bitcoin/pull/28976)

🤔 murchandamus reviewed a pull request: "wallet: Fix migration of blank wallets"
(https://github.com/bitcoin/bitcoin/pull/28976#pullrequestreview-1854840359)
Post merge ACK

🚀 achow101 merged a pull request: "init: settings, do not load auto-generated warning msg"
(https://github.com/bitcoin/bitcoin/pull/29301)

💬 achow101 commented on pull request "test: fix intermittent failure in p2p_v2_earlykeyresponse":
(https://github.com/bitcoin/bitcoin/pull/29352#issuecomment-1920002096)
ACK 9642aefb81a9c87227eccf9997380024247ed1fc

💬 achow101 commented on pull request "fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses":
(https://github.com/bitcoin/bitcoin/pull/26859#issuecomment-1920010176)
ACK b851c5385d0a0acec4493be1561cea285065d5dc

🚀 achow101 merged a pull request: "test: fix intermittent failure in p2p_v2_earlykeyresponse"
(https://github.com/bitcoin/bitcoin/pull/29352)

💬 hernanmarino commented on pull request "assumeutxo, rpc: Add 'start' parameter to loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/28659#issuecomment-1920019109)
> Ok, maybe turn it into draft, until you are done and the CI is passing?

Actually, I 'd like to ask you and @pablomartin4btc (who also reviewed) a question . I am convinced now to follow @ryanofsky ( and @Sjors ) suggestion of implementing this with 3 commands instead of the current approach of subcommands.

If you agree with this new approach , I think I'll close this PR and implement the functionality in one (or two) followup PRs implementing the new commands, since there is no point in
...

💬 furszy commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1473503275)
ok noted, thanks!

🚀 achow101 merged a pull request: "fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses"
(https://github.com/bitcoin/bitcoin/pull/26859)

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