🤔 furszy reviewed a pull request: "test: bugfix, synchronize indexes synchronously"
(https://github.com/bitcoin/bitcoin/pull/28026#pullrequestreview-1515410110)
> > (An alternative, functionally equivalent to this, might be to just make the timeout infinite, see [#27988 (comment)](https://github.com/bitcoin/bitcoin/pull/27988#issuecomment-1619218007), but I think this is fine as well.)
>
> I think I'd prefer that option because then we wouldn't need to add a test-only arg to `BaseIndex::Start`, plus having the same thread structure as in production seems more natural and more robust with respect to possible future changes of the init sequence and uni
...

💬 jonatack commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1253680561)
> test/functional/test_framework/test_node.py:7:1: F401 'asyncio' imported but unused

Agree, I see that as well with Python 3.11.4.

👋 mzumsande's pull request is ready for review: "net: Make AddrFetch connections to fixed seeds"
(https://github.com/bitcoin/bitcoin/pull/26114)

💬 jonatack commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1253688687)
Looking at this again, the following would move the string interpolation outside the loop (with perhaps clearer naming). Feel free to ignore.

```diff
- us_addrport = p2p_conn._transport.get_extra_info("socket").getsockname()
- info = [p for p in self.getpeerinfo() if p["addr"] == f"{us_addrport[0]}:{us_addrport[1]}"]
+ sockname = p2p_conn._transport.get_extra_info("socket").getsockname()
+ our_addr_and_port = f"{sockname[0]}:{sockname[1]}"
+
...

💬 mzumsande commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#issuecomment-1622575068)
Did some more manual testing and it worked as intended for me - moved out of draft!

💬 jonatack commented on pull request "Supporting parameter "h" and "?" in -netinfo.":
(https://github.com/bitcoin/bitcoin/pull/27830#discussion_r1253696102)
I don't have a strong opinion for or against on this proposed change, but the documentation for this user interface is provided by `bitcoin-cli -help`

```
$ ./src/bitcoin-cli -help | grep -A4 netinfo
-netinfo
Get network peer connection information from the remote server. An
optional integer argument from 0 to 4 can be passed for different
peers listings (default: 0). Pass "help" for detailed help
documentation.
```

and so this change would involve upd
...

💬 jonatack commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#issuecomment-1622588498)
Concept ACK, will have a look at this (and merged https://github.com/bitcoin/bitcoin/pull/27577 soon).

💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253703097)
Oh, yuck; that's a bad rebase. :(

💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253707986)
That's moving a member of a local struct/object, not a simple local, so it can't have been constructed in place and copy/move elision isn't possible, though?

💬 jonatack commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253709698)
Ah, you may be right.

https://shaharmike.com/cpp/rvo/#returning-member

💬 jonatack commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253739563)
<details><summary>If this test is valid (arm64 clang 16), returning the struct member without std::move may be better</summary><p>

```cpp
// rvo.cpp

#include <iostream>

struct Snitch { // Note: All methods have side effects
Snitch() { std::cout << "ctor" << std::endl; }
~Snitch() { std::cout << "dtor" << std::endl; }

Snitch(const Snitch&) { std::cout << "copy ctor" << std::endl; }
Snitch(Snitch&&) { std::cout << "move ctor" << std::endl; }

Snitch& operato
...

💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253743213)
For wtxid relay peers the prior `.insert(hash)` line adds the wtxid to the filter; for non-wtxid relay peers, it adds the txid to the filter. We only need to do a second insert to add the txid for wtxid relay peers, and then only when the wtxid is actually different to the txid. The extra check is just because testing a bool is easier than comparing two uint256s.

📝 achow101 reopened a pull request: "RPC/Wallet: Add "use_txids" to output of getaddressinfo"
(https://github.com/bitcoin/bitcoin/pull/22693)
(Non-GUI part of #15987, but without the bloom stuff)

💬 luke-jr commented on pull request "RPC/Wallet: Add "use_txids" to output of getaddressinfo":
(https://github.com/bitcoin/bitcoin/pull/22693#discussion_r1253745180)
That would break the wallettool again

💬 luke-jr commented on pull request "RPC/Wallet: Add "use_txids" to output of getaddressinfo":
(https://github.com/bitcoin/bitcoin/pull/22693#issuecomment-1622665055)
Rebased.

>if I understand correctly, this PR only makes it easier to tell if you're reusing one of your own wallet addresses for receiving, it doesn't tell you if you're reusing an address you've already used to send to someone else.

> do you want to have the "use_txids" for all addresses throughout the whole blockchain or only for a subset of them?

As documented, it only keeps track of addresses used in wallet transactions (whether we are the sender or receiver).

👋 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