👍 TheCharlatan approved a pull request: "kernel: Streamline util library"
(https://github.com/bitcoin/bitcoin/pull/29015#pullrequestreview-2055052953)
Re-ACK b58156701ac132f87b8ef8da1c7d22158c804a81

Good improvements since my last review and having some form of external verification for the work being done is very nice. If other reviewers or maintainers are not happy with introducing a bash script, I could volunteer some time to translate it.

The includes could be cleaned up a bit, especially `util/string.h` and `util/strencodings.h` are missing in a bunch of places. It would also be nice if the newly introduced files had correct include
...

💬 TheCharlatan commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1599843905)
I think rather than `test/` this should go into `contrib/devtools`, which contains functionally similar scripts like `symbol-check`, or `security-check`.

💬 hebasto commented on issue "`WalletCreate{Encrypted/Plain}` benchmark crash on Windows":
(https://github.com/bitcoin/bitcoin/issues/29816#issuecomment-2115302513)
My research shows that https://github.com/bitcoin/bitcoin/blob/dd42a5ddea6a72e1e9cad54f8352c76b0b701973/src/bench/wallet_create.cpp#L43-L44 does not trigger the `shared_ptr<CWallet>` deleter, which is `ReleaseWallet()`, because `wallet.use_count() == 3`.

💬 furszy commented on issue "`WalletCreate{Encrypted/Plain}` benchmark crash on Windows":
(https://github.com/bitcoin/bitcoin/issues/29816#issuecomment-2115320208)
> My research shows that
>
> https://github.com/bitcoin/bitcoin/blob/dd42a5ddea6a72e1e9cad54f8352c76b0b701973/src/bench/wallet_create.cpp#L43-L44
>
> does not trigger the `shared_ptr<CWallet>` deleter, which is `ReleaseWallet()`, because `wallet.use_count() == 3`.

Great 👍🏼 . #30122 releases the wallet too.

💬 ajtowns commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#issuecomment-2115335569)
> > I'm not sure how to be confident that the UpdateBlockTipSync / hashRecentRejectsChainTip would be correct and didn't miss an edge case.
>
> I (locally) split the commit into (1) update on validation interface callback and asserting that `hashRecentRejectsChainTip` is equal to the chain tip whenever we call `AlreadyHaveTx` + (2) removing `hashRecentRejectsChainTip`. I think if we see that everything runs fine with (1) it's probably correct.
>
> I've (locally) hit a bug though, so will f
...

👍 ryanofsky approved a pull request: "kernel: De-globalize fReindex"
(https://github.com/bitcoin/bitcoin/pull/29817#pullrequestreview-2060872465)
Code review ACK b47bd959207e82555f07e028cc2246943d32d4c3. I rereviewed the whole PR, but the only change since last review was reverting the bugfix https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1578327024 and make the change a pure refactoring.

I think this change makes the code a lot less confusing by using separate names for the -reindex option and reindexing state, and could avoid bugs like the one mentioned in the future.

💬 rkrux commented on pull request "test: Added test to ensure log and failure happen when work is less than active chainstate":
(https://github.com/bitcoin/bitcoin/pull/30105#issuecomment-2115345439)
Just realised this is similar to this PR: https://github.com/bitcoin/bitcoin/pull/29428

💬 fanquake commented on pull request "bench: enable wallet creation benchmarks on all platforms":
(https://github.com/bitcoin/bitcoin/pull/30122#issuecomment-2115353538)
It'd be good if the commit message could explain why this is being done. It seems like the current approach is just to try and refactor away the Windows bug, even though it might not have been properly identified (prior to https://github.com/bitcoin/bitcoin/issues/29816#issuecomment-2115302513). However now that the bug might have been found, it'd be good to at least document what it is, and if possible, fix/workaround it in a more targetted way, rather than refactor code that is currently perfe
...

⚠️ ensoimaxim7 opened an issue: "I"
(https://github.com/bitcoin/bitcoin/issues/30123)

✅ fanquake closed an issue: "I"
(https://github.com/bitcoin/bitcoin/issues/30123)

:lock: fanquake locked an issue: "I"
(https://github.com/bitcoin/bitcoin/issues/30123)

✅ achow101 closed a pull request: "p2p: Fill reconciliation sets (Erlay)"
(https://github.com/bitcoin/bitcoin/pull/28765)

💬 achow101 commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#issuecomment-2115368489)
Superseded by #30116

💬 jonatack commented on pull request "[26.x] archive 26.1 release notes + backports":
(https://github.com/bitcoin/bitcoin/pull/29899#issuecomment-2115368797)
Propose the first commit of #30085 for backport.

✅ ryanofsky closed an issue: "Support JSON-RPC 2.0"
(https://github.com/bitcoin/bitcoin/issues/2960)

🚀 ryanofsky merged a pull request: "Support JSON-RPC 2.0 when requested by client"
(https://github.com/bitcoin/bitcoin/pull/27101)

💬 glozow commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#issuecomment-2115386677)
Was it mempool_packages.py maybe? Mine tripped there on `invalidateblock` when I was adding `UpdatedBlockTip` to `InvalidateBlock`, and it was a lock ordering problem.

Hm. The alternative is to hold `cs_main` and tell txdownloadman the current chain tip pretty much all the time...

💬 furszy commented on pull request "bench: enable wallet creation benchmarks on all platforms":
(https://github.com/bitcoin/bitcoin/pull/30122#issuecomment-2115413888)
> It'd be good if the commit message could explain why this is being done. It seems like the current approach is just to try and refactor away the Windows bug, even though it might not have been properly identified (prior to [#29816 (comment)](https://github.com/bitcoin/bitcoin/issues/29816#issuecomment-2115302513)). However now that the bug might have been found, it'd be good to at least document what it is, and if possible, fix/workaround it in a more targetted way, rather than refactor code t
...

👍 rkrux approved a pull request: "test: improve robustness of connect_nodes()"
(https://github.com/bitcoin/bitcoin/pull/30118#pullrequestreview-2060988698)
tACK [59ba873](https://github.com/bitcoin/bitcoin/pull/30118/commits/59ba8735102aebd456bb9dc831759c82e95763c0)

Make successful, so are all the functional tests.

Overall I agree with this change because it makes the nodes connection verification dependent more on `connect_nodes()` arguments `a, b` instead of the earlier approach that relied more on the state of the whole response of `getpeerinfo()`, which seemed brittle as mentioned in the PR description.

@furszy Were you able to identif
...

💬 rkrux commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1603498152)
`find_conn(from_connection, to_connection_subver, inbound=False)`

Might as well make these calls once and store in variable instead of finding 3 times? Unless I'm missing something that requires these calls to be made every time.