💬 jonatack commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253479064)
Unrelated to this pull, but seems odd that `entry_height` is set to 0 here when 1 is the usual initial value.

💬 jonatack commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253438163)
eb4dc54 Are these special member functions [needed](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-zero), and if yes, [declare them all?](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c21-if-you-define-or-delete-any-copy-move-or-destructor-function-define-or-delete-them-all) A comment in the commit explaining why they are added would be helpful.
```diff
+ CTxMemPoolEntry() = delete;
+ ~CTxMemPoolEntry() = default;
CTxMemPoolEntry(const CTxMemPoolEnt
...

💬 jonatack commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253501078)
ca6379eb While touching this, with guaranteed copy elision it's now almost always a pessimization to return `std::move(local)` (and the other remaining return values in this function are `return it->second;` and `{}`).

```suggestion
return txinfo.tx;
```

💬 jonatack commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253541014)
https://github.com/bitcoin/bitcoin/commit/ca6379eba56ca8f669fea6c91877e9ecbb31451b why is `peer->m_wtxid_relay` added in this conditional? (in which case the comment just below would need updating)

💬 jonatack commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253548429)
https://github.com/bitcoin/bitcoin/commit/ca6379eba56ca8f669fea6c91877e9ecbb31451b It looks like the documentation above needs updating, i.e. `s/current_time/current sequence/`, now that `tx_relay->m_last_inv_send_time = SteadyClock::now();` in this commit was changed to `tx_relay->m_last_inv_sequence = m_mempool.GetSequence();`

💬 jonatack commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253614165)
https://github.com/bitcoin/bitcoin/commit/eb4dc54b3c9b9edad9b469b07cbac0573bbf1a37 A Doxygen doc explaining the `for_relay` aspect and `last_sequence` logic would be handy.

💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1253452419)
Okay, will be reverting to `new_total_fee`

💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1253624315)
I’ve been doing some staring at this today. It seems to me that the waste score of finished input sets should also be adjusted according to the bump fee discount. Gotta do more pondering, will get back to this.

💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1253454337)
Yes, thanks, I removed the earlier instances of ComputeAndSetWaste where the results get returned from the various coin selection algorithms and instead of selectively updating here, always calculate it here now. I just forgot to remove the comment.

🤔 achow101 reviewed a pull request: "Support JSON-RPC 2.0 when requested by client"
(https://github.com/bitcoin/bitcoin/pull/27101#pullrequestreview-1515315141)
I think we aren't handling missing `id`s correctly?

From the spec:

> id
> An identifier established by the Client that MUST contain a String, Number, or NULL value if included. If it is not included it is assumed to be a notification. The value SHOULD normally not be Null [1] and Numbers SHOULD NOT contain fractional parts [2]

When there is no `id`, we should treat the as a notification and can just ignore the request, rather than responding to it as we currently do. It seems like i
...

💬 achow101 commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1253614618)
In d11fb70b3fea7c1be5f48e32837d2c81dc7cbd10 "test: cover more HTTP error codes in interface_rpc.py"

This seems unnecessary?

💬 jonatack commented on pull request "wallet: Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#issuecomment-1622495909)
@pinheadmz I'd do the simplest thing, or what @achow101 thinks on this and regarding progress on the https://github.com/bitcoin/bitcoin/issues/20160 timeline.

🤔 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. :(