💬 furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#issuecomment-1851051245)
Updated per feedback. Thanks @murchandamus.
Test-only changes. Removed the assumption that BnB always executes first.

🤔 ajtowns reviewed a pull request: "kernel: Streamline util library"
(https://github.com/bitcoin/bitcoin/pull/29015#pullrequestreview-1776320307)
Okay, I think I'm on roughly the same page:

* keeping Split in util make sense
* moving our string stuff into the util namespace seems fine * moving the spanparsing stuff to script mostly makes sense -- though I feel a bit like mixing consensus critical script manipulation and miniscript/descriptor parsing in the same namespace is a bit clunky?
* moving fees.h/error.h to "common" makes sense; though if we can make "common" just be a broader part of "util" that seems better long term.
*
...

💬 ajtowns commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1423218395)
Doesn't libconsensus necessarily depend on libcrypto (for sha256 at least)?

💬 ajtowns commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1423227364)
It seems odd to put this in "node" when `TransactionErrorString(TransactionError error)` is in "common" ?

`TransactionError` is returned by `CombinePSBTs()` which seems like something that could reasonably be exposed by a command line tool without needing a full node setup.

So putting this in common seems more sensible to me?

🤔 amitiuttarwar reviewed a pull request: "p2p: attempt to fill full outbound connection slots with peers that support tx relay"
(https://github.com/bitcoin/bitcoin/pull/28538#pullrequestreview-1776309045)
code looks good to me. left a comment about the test that seems worth fixing, then I'm ready to ACK the patch.

trying to think about any potential negative network effects that could occur, but so far it seems fine to disconnect OB full relay peers that signal they don't want to receive transactions.

💬 amitiuttarwar commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1423210717)
```suggestion
self.log.info("Check that we sustain full-relay outbound connections to blocksonly peers when not at the full outbound limit")
```

💬 amitiuttarwar commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1423245744)
imo users should get an error if they set `-maxconnections` to less than 8, or at least a "this is unsafe. are you SURE?!". but that's probably getting off topic here :)

💬 amitiuttarwar commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1423241016)
I don't think this is testing the expected behavior. The test node is currently started up in `blocksonly` mode, so won't enter the new conditional in `EvictExtraOutboundPeers` regardless of the `extra_outbound_count` check. To get the test to fail, I added a restart. So right now, this 1st new test and the 3rd new test are the same thing.

My suggestion is to break these 3 new tests into a separate subtest that restarts with `-noblocksonly` to help make it clear what the preconditions are.

...

💬 amitiuttarwar commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1423252977)
updating `ForEachNode` to no longer check `NodeFullyConnected` seems pretty intrusive & like all callers would need to be carefully checked, so imo it doesn't seem super valuable to repeat the check here. but cost of redundant check is also pretty low so doesn't seem like a big deal either way.

💬 amitiuttarwar commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1423253383)
maybe `GetFullOutboundDelta` ?

💬 theStack commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#discussion_r1423278081)
nit: maybe '?' would also work to express the "detecting" state. But no hard feelings, typically users wouldn't see this a lot anyway.

🤔 theStack reviewed a pull request: "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo"
(https://github.com/bitcoin/bitcoin/pull/29058#pullrequestreview-1776407268)
Concept ACK

Playing devil's advocate: could there be any noticeable drawbacks of always trying with v2 first? I'm thinking of destination nodes behind firewalls with deep packet inspection which block ips from further connections if they don't follow the protocol (i.e. for bitcoin v1 connections, initiating with a version message). If that's the case, the user would only have the option to disable v2transport in general, or use the `addnode` RPC instead. Not sure how realistic that "i want to
...

💬 theStack commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#discussion_r1423276132)
nit: can the comment above ("... does not currently have a way to signal BIP324 support") now also be removed? It's content is still true, but with the "always attempt v2 first if we support it"-approach, I guess it's not relevant anymore.

👍 theStack approved a pull request: "Replace non-standard CLZ builtins with c++20's bit_width"
(https://github.com/bitcoin/bitcoin/pull/29057#pullrequestreview-1776458843)
Code-review ACK 273e4dc8da41f113b21a7ece54988b7bddf3612e

Unit tests succeeded locally :heavy_check_mark:

Seems reasonable to keep `CountBits` in a first step with its tests and fuzzers for reassurance and then replace all of its call-sites. Happy to re-review this PR or a follow-up, depending on where you decide to put the next commit.

💬 mzumsande commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#discussion_r1423315657)
Heh, I actually changed it from ? to * last thing before opening the PR, because * was already used in a few other places for netinfo and ? would be a new symbol. But maybe ? is more natural here, @jonatack do you have an opinion?

💬 murchandamus commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1423327495)
I see what happened. Knapsack is finding a changeless solution in the first attempt where it should look for something with change because `min_change_target` was set to zero

🤔 murchandamus reviewed a pull request: "wallet: skip BnB when SFFO is enabled"
(https://github.com/bitcoin/bitcoin/pull/28994#pullrequestreview-1776489114)
ACK 18669d753a15f997f7363dcaf0abf230b851b224

💬 murchandamus commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1423332614)
Setting `min_change_target` to 0 makes Knapsack always look for changeless solutions

💬 furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1423339033)
Fixed. Thanks.

🤔 murchandamus reviewed a pull request: "wallet: skip BnB when SFFO is enabled"
(https://github.com/bitcoin/bitcoin/pull/28994#pullrequestreview-1776504690)
ACK 18669d753a15f997f7363dcaf0abf230b851b224