💬 jonatack commented on pull request "I2P: also sleep after errors in Accept() & destroy the session if we get "Session was closed"":
(https://github.com/bitcoin/bitcoin/pull/28077#issuecomment-1636339566)
Initial code review ACK (didn't review the test changes yet), will test.

💬 ryanofsky commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#issuecomment-1636342363)
This is rebased after #27607, which allowed making a lot of simplifications and improvements. Also, I think the PR is better organized now, so I think I could split off the first commit or first few commits into a separate PR if reviewers would find that helpful.

💬 achow101 commented on pull request "descriptors: do not return top-level only funcs as sub descriptors":
(https://github.com/bitcoin/bitcoin/pull/28067#issuecomment-1636345229)
ACK 47abfe1525cc2700699ba0605747f1ad36a34a1b

💬 ryanofsky commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#issuecomment-1636352480)
> The PR description is outdated - its last sentence can be removed.

Thanks, struck it out since PR was merged and the old description did get added to git history.

💬 Sjors commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#issuecomment-1636359937)
0ffa0f1c370494b6b99a9cdf911b20ee655ac829 (and later) breaks at line line 96 when you run `wallet_backwards_compatibility.py --descriptors` without BDB:

```
JSONRPCException: Wallet file verification failed. Failed to open database path '/tmp/bitcoin_func_test_x3n9_0vn/node1/regtest/wallets/w1_v19'. Build does not support Berkeley DB database format. (-18)
```

Otherwise it works, just some stylistic notes inline.

> 0.16 creating new wallets rather than testing the target wallets

Is
...

💬 Sjors commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1264084847)
6a5d245b89c51a53989b984d31d7bb4d807837c3: if a node is node is not used, we should reduce the number of nodes and `node_v19` should be `self.num_nodes - 3`.

💬 Sjors commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1264088086)
72362ed1fbbab9dfbaf224f930ed9b328f29c55f: maybe keep all the `node_v..` definitions in one place at the top of the test?

💬 Sjors commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1264078873)
c31e0db7f4263096d503f07707af9d0a733e50da The original `< 170000` made it more clear that this applies to _all_ old versions. Not a big deal though, since it's unlikely we'll add even older nodes.

💬 achow101 commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1264119254)
0.18 is still tested, just not explicitly with its own special casing and variables.

💬 achow101 commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1264136631)
No, this is more correct. This behavior doesn't apply to all older versions, only to 0.16.x. Versions prior to this kept the wallet files in the main datadir. 0.16 added the wallet directory but the wallets are still per file. 0.17 moved wallets into the current one directory per wallet structure.

🤔 mzumsande reviewed a pull request: "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module"
(https://github.com/bitcoin/bitcoin/pull/28031#pullrequestreview-1531022386)
Some thoughts about logging:

After testing this on mainnet for a bit, I think it would be nice to be able to follow the fate of each individual orphan in `BCLog::TXPACKAGES`:
We currently have a tx-level based log entry for addition (“stored orphan..."), but nothing for removal, so having additional log entries in `MempoolAcceptedTx()` for succesful resolution and `MempoolRejectedTx()` for failure would be nice, maybe also a tx-level message for removal due to expiration and overflow.

If
...

💬 achow101 commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1264170579)
Instead of using magic numbers, I've changed the test to use helper functions that check the major version.

💬 achow101 commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1264170660)
Done

💬 achow101 commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#issuecomment-1636440917)
> [0ffa0f1](https://github.com/bitcoin/bitcoin/commit/0ffa0f1c370494b6b99a9cdf911b20ee655ac829) (and later) breaks at line line 96 when you run `wallet_backwards_compatibility.py --descriptors` without BDB:

Fixed

> > 0.16 creating new wallets rather than testing the target wallets
>
> Is there an assert you can add in [c31e0db](https://github.com/bitcoin/bitcoin/commit/c31e0db7f4263096d503f07707af9d0a733e50da) that would catch this directly? Right now a regression would be "caught" simp
...

💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1264177843)
Done.

💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1264177911)
Done, thanks!

💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1264178108)
I think it's just a nice thing to do to ensure that all state is reset when the block index is cleared out and re-initialized; it might be surprising if state were leftover? But I agree that it doesn't seem strictly necessary. Added a comment to clarify.

💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1264178256)
Updated the commit message.

💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1264178307)
I'd say mostly for realism. My intuition for how this is designed is that if you don't reset the HAVE_DATA flags, then you'd immediately expect the background chain to process and validate all the blocks for which we HAVE_DATA and thus arrive at the original tip, as soon as you call ActivateBestChain().

💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1264179053)
We need this due to the CheckBlockIndex() invariant that you can't have a block index entry with nTx > 0 and without HAVE_DATA unless you've either pruned or used ASSUMED_VALID. Added a comment to clarify.