👋 luke-jr's pull request is ready for review: "RPC/Wallet: Add "use_txids" to output of getaddressinfo"
(https://github.com/bitcoin/bitcoin/pull/22693)

💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253748543)
Updated

💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253751283)
I think the test you want is `auto s = Wrapper(); return std::move(s.snitch);`

💬 jonatack commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253753580)
You're right, thanks.

```
foo_move...
ctor
move ctor
dtor

foo...
ctor
copy ctor
dtor
```

💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1622681555)
Updated for jonatack's review -- removes unneeded code changes, updates comments.

🤔 theStack reviewed a pull request: "Add support for RFC8439 variant of ChaCha20"
(https://github.com/bitcoin/bitcoin/pull/27985#pullrequestreview-1515520041)
Concept ACK

💬 theStack commented on pull request "Add support for RFC8439 variant of ChaCha20":
(https://github.com/bitcoin/bitcoin/pull/27985#discussion_r1253759518)
tiny nit (probably only worth changing if you have to retouch): could check this assert unconditionally, to not allow test cases where `hex_input` is empty but `hex_expected_output` is not

💬 theStack commented on pull request "test: refactor: deduplicate legacy ECDSA signing for tx inputs":
(https://github.com/bitcoin/bitcoin/pull/28025#discussion_r1253771758)
Seems like the input index is checked in `LegacySignatureMsg` (which is in turn called by `LegacySignatureHash`): https://github.com/bitcoin/bitcoin/blob/bc4f6b13feb29146b7e10e86f93dc7f6fb6937f2/test/functional/test_framework/script.py#L638-L639
For `sighash_type` it might be nice to add a type annotation, though I don't know if the byte range (0..255) can be specified exactly (currently that value is not set explicitly, as all our call-sites use the default SIGHASH_ALL). As for `tx.vin[input_i
...

💬 theStack commented on pull request "test: refactor: deduplicate legacy ECDSA signing for tx inputs":
(https://github.com/bitcoin/bitcoin/pull/28025#issuecomment-1622720582)
> Also - do you think we need a segwitV0 function like this as well? As far as I can grep there's only 2 such signing operations in `p2p_segwit.py` so maybe that's more work than its worth.

Good point -- I think it would be nice to introduce similar functions for other signature types (e.g. `sign_input_segwitv0`) for consistency reasons. Might be worth a follow-up, together with some other improvements pointed out in https://github.com/bitcoin/bitcoin/pull/28025#discussion_r1253580835 (type a
...

💬 ajtowns commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1253796131)
This implementation of that logic doesn't seem ideal to me:

* sending the TX without doing INV/GETDATA first risks having the TX be ignored as unrequested

* `PushUnbroadcastTx` doesn't actually send the TX, but `MaybeSendPing` does actually send the PING, so you might get the PONG and closes the connection before the other end has actually received/processed the TX

💬 jonatack commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1253806859)
CI failing at this line.

https://cirrus-ci.com/task/5531648075235328?logs=ci#L3397
https://cirrus-ci.com/task/5531648075235328?logs=ci#L4424

`Expected messages "['DumpAnchors: Flush 1 outbound block-relay-only peer addresses to anchors.dat']" does not partially match log:`

```
- 2023-07-05T18:23:01.237732Z [shutoff] [logging/timer.h:56] [Log] DumpAnchors: Flush 0 outbound block-relay-only peer addresses to anchors.dat started
- 2023-07-05T18:23:01.286842Z [shutoff] [logging/timer.h
...

💬 jonatack commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1253818208)
Reproduced this test failure locally on the 4th run of `test/functional/feature_anchors.py` (arm64 macOS 13.4.1)

💬 ajtowns commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1253827052)
We already require `NET_*` to go from 0 to NET_MAX with no gaps every time we do `for (int n = 0; n < NET_MAX; ++n)`. It's a standard expectation of enums and it's documented as such in netaddress.h. It's not an added requirement, and doesn't make the code fragile or harder to change...

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1253855939)
reverted to the original optional arg in the latest push (https://github.com/bitcoin/bitcoin/compare/7a1c663e3727695a081a735c37819241341f3730..1e65aae8068ecf313a7c3b176dfc1326b3b67fbe) - checking for `preferred_net` meant I changed the default behavior. in order to fix that I would have had to add another else statement which seems more confusing than the original implementation, so went back

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1622876973)
small change to revert the function signature `MaybePickPreferredNetwork` to have network be an optional. can see https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1253855939 if you're interested in more detail.

💬 ryanofsky commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1253898394)
In commit "make GetFirstStoredBlock assert that 'start_block' always has data" (aad83e489a9f515a1da36acd3e99f35b1a0da53c)

The `pruneblockchain` RPC doesn't consider it an error to prune a block that's already been pruned. It just makes a best effort to prune what it can, and then it returns the last height of block without transaction data.

So I think it would be more consistent to not make this an error, and just do:

```c++
PruneBlockFilesManual(active_chainstate, height);
return blo
...

💬 ryanofsky commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1253906502)
In commit "make GetFirstStoredBlock assert that 'start_block' always has data" (aad83e489a9f515a1da36acd3e99f35b1a0da53c)

This preserves current behavior, but it is inconsistent with the documentation which says the field is be set to "height of the last block pruned, plus one". To make the code actually consistent with the documentation, this should be:

```c++
obj.pushKV("pruneheight", has_tip_data ? chainman.m_blockman.GetFirstStoredBlock(tip)->nHeight : tip.nHeight + 1);
```

Altern
...

💬 ryanofsky commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1253746784)
In commit "make GetFirstStoredBlock assert that 'start_block' always has data" (aad83e489a9f515a1da36acd3e99f35b1a0da53c)

It seems like there is a slight change in behavior here. Previously if the tip did not have data, but all blocks before it did have data there would not be a prune violation. Now there will be prune violation.

This is probably a good thing. I don't think this case actually needs to be an error if the missing blocks are <= than the assumeutxo snapshot height, and the blo
...

💬 ryanofsky commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1253746547)
In commit "make GetFirstStoredBlock assert that 'start_block' always has data" (aad83e489a9f515a1da36acd3e99f35b1a0da53c)

re: https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1248314125

> I think that the changes are minimal and help to reduce confusion. But let me know what you guys think.

Thanks for that. I think it would have been fine to just keep the `GetFirstStoredBlock` behavior unchanged, and just document it, but fixing the call sites is probably better in the long run
...

💬 S3RK commented on pull request "wallet: Give deprecation warning when loading a legacy wallet":
(https://github.com/bitcoin/bitcoin/pull/27869#issuecomment-1623103601)
reACK 8fbb6e99bfc85a1b9003cae402a7335843a86abd