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

💬 rkrux commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1603491415)
Looking at the sample output of `subversion` - `"subversion": "\/Satoshi:25.1.0\/"`, it doesn't seem unique enough because multiple nodes can be running the same version. Won't this cause issues in `find_conn` later?

https://chainquery.com/bitcoin-cli/getnetworkinfo

💬 ajtowns commented on pull request "policy: restrict all TRUC (v3) transactions to 25KvB":
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2115459510)
I think the 100kB value for transaction relay was adopted as follows:

* 2010-09-13 In 3df62878c3c satoshi added a 100kB (`MAX_BLOCK_SIZE_GEN/5` with `MBS_GEN = MAX_BLOCK_SIZE/2`) limit on new transactions in `CreateTransaction()`
* 2013-02-04 #2273 In gavin gave that constant a name, and made it apply to transaction relay as well

So I don't think there was any deep thought put into that figure. 25kvB sounds fine to me.

Given the suggestion in the OP that it's easier to increase the v
...

👍 ryanofsky approved a pull request: "blockstorage: Separate reindexing from saving new blocks"
(https://github.com/bitcoin/bitcoin/pull/29975#pullrequestreview-2061065588)
Code review ACK e41667b720372dae8438ea86e9819027e62b54e0. Just improvements to comments since last review.

💬 ryanofsky commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1603529683)
In commit "blockstorage: split up FindBlockPos function" (064859bbad6984a6ec85c744064abdf757807c58)

note: At this point in the PR, GetSerializeSize is already called by the SaveBlockToDisk function calling it, so calling it again here is a bit inefficient. But this is resolved by the next commit moving the the UpdateBlockInfo call

👍 stickies-v approved a pull request: "[refactor] Check CTxMemPool options in ctor"
(https://github.com/bitcoin/bitcoin/pull/28830#pullrequestreview-2061046449)
ACK 856c8563ca342303a83977146df22768bb6e2c7e