🤔 pinheadmz reviewed a pull request: "Relay own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/27509#pullrequestreview-1555409244)
code review re-ACK 2541f09439099ec3e73f7c5a12f809f190e6af1d

confirmed changes since last ack were rebase and small review comments addressed. Found a typo, and played with the feature on signet (longer report about that to follow)

💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1279655081)
nit: s/send/sent

💬 willcl-ark commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#discussion_r1279739808)
nit: should we `sys.exit(1)` here too?

👍 willcl-ark approved a pull request: "contrib: add tool to convert compact-serialized UTXO set to SQLite database"
(https://github.com/bitcoin/bitcoin/pull/27432#pullrequestreview-1376557345)
tACK 3ce180ac2d

Left two nits which don't need addressing unless being re-touched, but overall this works well in testing and seems like a useful contrib script. Converting the output to json also worked as described in the comments above.

💬 willcl-ark commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#discussion_r1160947835)
Whitepsace nit (unless it was deliberate to align with following line):

```suggestion
parser.add_argument('infile', help='filename of compact-serialized UTXO set (input)')
```

🤔 stickies-v reviewed a pull request: "ci: Integrate `bitcoin-tidy` clang-tidy plugin"
(https://github.com/bitcoin/bitcoin/pull/26296#pullrequestreview-1555593455)
Approach ACK 3a727cd7ee02c83efcd57d004e6fca8d8e1bb33b

Did a code review too but both the build stuff as well as the clang-tidy syntax is still very new to me so doesn't mean too much.

Are the tests being run in CI with this PR? And perhaps some tidy-test for multiline strings would be helpful too?

💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1659008138)
Ran with this feature on signet and found a few unexpected behaviors:

1. `-sensitiverelay=1` and `-onlynet=ipv4` should probably throw an init error. In this configuration, sensitive stuff seems to be ignored and new wallet TXs are relayed via ipv4 peers like normal. Is "only use tor for sensitive relay" a possible configuration?

2. Rapidly sending multiple transactions `"...incremented the number of connections to open from 20 to 25..."` this number grows rapidly and decreases slowly (esp
...

💬 achow101 commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1279730123)
In 29d9d326d5193bb9a410a8881eabc93de5dd6266 "[txorphanage] track size of stored orphans, total and by peer"

This function is unimplemented.

💬 achow101 commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1279714521)
In 543273d96e896adf5531ed961856aa0eb70cbe57 "[log] log ProcessOrphanTx() events by wtxid"

Perhaps log both txid and wtxid?

💬 achow101 commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1279743325)
In 974d864419dd98be6e32dec3ee11f5082b060b1b "[refactor] make TxPackageTracker responsible for EraseTx and AddChildrenToWorkset"

This line seems a bit unrelated to this commit as there is no pre-existing `EraseTx`.

💬 achow101 commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1279747416)
In b5ab45e595bbcedbd602b6385b83e9ffd983f216 "[p2p/refactor] make TxPackageTracker responsible for orphan resolution"

nit: indentation

👍 ryanofsky approved a pull request: "Rework validation logic for assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/27746#pullrequestreview-1555524219)
Code review ACK a733dd79e29068ad1e0532ac42a45188a040a7b9. Just suggested changes since the last review. There are various small things that could be followed up on, but I think this is ready for merge.

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1279760417)
In commit "Fix initialization of setBlockIndexCandidates when working with multiple chainstates" (768690b7ce551cd403f8e2a099372915f6022ad4)

Would be good to directly verify the snapshot block is included:

```c++
// block before the snapshot is excluded (as well as earlier blocks)
BOOST_CHECK_EQUAL(cs2.setBlockIndexCandidates.count(assumed_base->pprev), 0);
// snapshot block is included (as well as later blocks)
BOOST_CHECK_EQUAL(cs2.setBlockIndexCandidates.count(assumed_base), 1);
``
...

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1279811734)
re: https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1277557586

> [0ce805b](https://github.com/bitcoin/bitcoin/commit/0ce805b632dcb98944a931f758f76f530f5ce5f2): why isn't `BLOCK_VALID_TRANSACTIONS` also alternatively `ASSUMED_VALID`?

I'm not sure what this review comment is asking. The code comment above still seems accurate to me, but it maybe it could be improved.

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1279767675)
re: https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1278312526

> [768690b](https://github.com/bitcoin/bitcoin/commit/768690b7ce551cd403f8e2a099372915f6022ad4): this had me confused, but it's because the test considers a scenario where the snapshot base block has been downloaded. It could be nice to include the moment _before_ it's downloaded where this should be `0`.

This review comment does not match my understanding. The snapshot block is `assumed_base` at height 39 and it does
...

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1279727266)
re: https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1278308074

> `GetSnapshotBaseBlock()` is `nullptr` so the special-case won't apply

I may be misunderstanding, but I don't think it's true that `GetSnapshotBaseBlock` is null for the background chainstate. This is a ChainstateManager method, so `GetSnapshotBaseBlock` is going to be non-null if any snapshot is loaded, and this code will try to add the snapshot block to both chainstates, even though it's not needed for the backgrou
...

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1279781965)
In commit "Fix initialization of setBlockIndexCandidates when working with multiple chainstates" (768690b7ce551cd403f8e2a099372915f6022ad4)

I think it's a little misleading to say the other chainstate contains all block except those marked assume-valid because:

- It actually does contain one block marked assumed-valid, which is the snapshot base (block 39)
- It also excludes other blocks which are not assumed-valid because they don't have more work than the tip (blocks 1-19)

I think th
...

🚀 ryanofsky merged a pull request: "Rework validation logic for assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/27746)

🤔 furszy reviewed a pull request: "wallet: Have the wallet store the key for automatically generated descriptors"
(https://github.com/bitcoin/bitcoin/pull/26728#pullrequestreview-1554781243)
Just started, left few comments.

💬 furszy commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1279310998)
In 3de9672a:

Could verify here that when `num_ckeys > 0`, `num_keys == 0` and viceversa. Failing early if it is not true.

💬 furszy commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1279450800)
In 97493ab8:

What `best_time` is here?
Shouldn't this be looking for the oldest one?, e.g `(best_time == 0 || best_time > desc_time)`