💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1286421558)
Fixed

💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1668641951)
Force-pushed:

1. Renamed `whitelist_peers` to `noban_tx_relay` and set up it `True` for tests that explicity was whitelisting peers with this purpose of speeding up. - https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1274667131
2. Added test coverage for an outbound node that misbehaves and doesn't get banned. - https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1275440729
3. Updated `-blocksonly` documentation.
4. Changed `InitializePermissionFlags` to deal with `https://g
...

💬 brunoerg commented on pull request "fuzz: wallet, add target for Spend":
(https://github.com/bitcoin/bitcoin/pull/28236#discussion_r1286425941)
I think that `CreateSyncedWallet` will call `SetupDescriptorScriptPubKeyMans()` which internally will make a seed.
```cpp
void CWallet::SetupDescriptorScriptPubKeyMans()
{
AssertLockHeld(cs_wallet);

if (!IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) {
// Make a seed
CKey seed_key;
seed_key.MakeNewKey(true);
CPubKey seed = seed_key.GetPubKey();
assert(seed_key.VerifyPubKey(seed));

// Get the extended key
CExtKey maste
...

💬 brunoerg commented on pull request "fuzz: wallet, add target for Spend":
(https://github.com/bitcoin/bitcoin/pull/28236#discussion_r1286426350)
s/chose/choose

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1286427458)
> This interface is not the right place for a callback like this because it is not a "net event".

Hmm ok, that would avoid locking `cs_main` in the open connections thread. Which is nice.

💬 brunoerg commented on pull request "fuzz: wallet, add target for Spend":
(https://github.com/bitcoin/bitcoin/pull/28236#discussion_r1286427767)
I think you could keep same pattern as you had previously:
```cpp
std::vector<int> random_positions = {-1, fuzzed_data_provider.ConsumeIntegralInRange<int>(0, recipients_size), fuzzed_data_provider.ConsumeIntegral<int>()};
int change_pos = PickValue(fuzzed_data_provider, random_positions);
```

instead of
```cpp
int random_positions[3] = {-1, fuzzed_data_provider.ConsumeIntegralInRange<int>(0, mtx.vout.size()), fuzzed_data_provider.ConsumeIntegral<int>()};
int change_pos = fuzzed_data_
...

💬 luke-jr commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1668653252)
>Simply claiming this doesn't seem helpful. What is the compile error you encountered? (Ideally with steps to reproduce)

The several issues bootstrapping Rust are well-known and documented elsewhere. Fixing it is not within the scope of this project. It simply shouldn't be relied on until then.

💬 luke-jr commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1668673685)
1c1bd6cbc6d96fff7f67186aeb57adfe0ee872c4 doesn't build

💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1668682243)
> https://github.com/bitcoin/bitcoin/commit/1c1bd6cbc6d96fff7f67186aeb57adfe0ee872c4 doesn't build

sorry, fixing!

💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1668690479)
> https://github.com/bitcoin/bitcoin/commit/1c1bd6cbc6d96fff7f67186aeb57adfe0ee872c4 doesn't build

Force-pushed fixing it

💬 luke-jr commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1286463105)
nit: Dislike scattering this all over the place. `node_services` doesn't even have a meaning with nullptr. Maybe return `std::optional<...>` instead?

💬 pajasevi commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1668715092)
Sorry @1ma but I must correct you because your premise is wrong:
> If BIP47 v3 took off and there were tens of millions of BIP47 v3 notification transactions (which would arguably be a good thing for privacy) it would also contribute to worsen the UTXO set bloat problem for no good reason.

v3 notification transactions do not bloat the UTXO set because they are meant to be spent after they are made. Citing from the [OBPP5](https://github.com/OpenBitcoinPrivacyProject/rfc/blob/master/obpp-05.m
...

💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1286472092)
Seems more elegant.

💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1668722534)
Thanks, @luke-jr! Force-pushed changing `CConnman::ConnectNode` to std::optional

💬 theStack commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#discussion_r1286479195)
FWIW, I agree that the `Done` prefix of the method name is quite confusing, for both the reasons Sjors lined out. `CanSetNewMessageToSend()` sounds much better to me (maybe even just `CanSetMessageToSend`, to have the full name of the method included which can be called if `true` is returned?).

🤔 theStack reviewed a pull request: "net: transport abstraction"
(https://github.com/bitcoin/bitcoin/pull/28165#pullrequestreview-1566293438)
Concept ACK

Slowly working my way through, reviewed up to f97adbf3e93e89b1e8ce1dc212e84ac6b2879463 (commit 3/8), looks good so far.
One potential code deduplication nit (though I'm not sure if it gains that much in readability): seems like for V1Transport, `HaveBytesToSend()` and `DoneSendingMessage()` are in an inverse relation (or however one would call that), so one could be expressed through the other, e.g.:
```diff
index 887669e32b..af0fa5c603 100644
--- a/src/net.cpp
+++ b/src/net
...

💬 ariard commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1286479898)
Fair, I still think it’s slightly less DoSy though depends benchmark and UTXO layout on disk of course.

💬 theStack commented on pull request "crypto: more `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#issuecomment-1668735071)
Concept ACK

💬 ariard commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1286486164)
could say “low-fanout flooding” as it’s the term used above to describe the two transaction announcement strategy

💬 brunoerg commented on pull request "fuzz: wallet, add target for Spend":
(https://github.com/bitcoin/bitcoin/pull/28236#issuecomment-1668752812)
From lint:
```diff
+ test/lint/all-lint.py
This diff appears to have added new lines with trailing whitespace.
The following changes were suspected:

diff --git a/src/wallet/test/fuzz/spend.cpp b/src/wallet/test/fuzz/spend.cpp
@@ -0,0 +1,254 @@
+namespace wallet {
+ * Singleton wallet class ensures that only one
+ * instance of CWallet is created and reused as required.
+ * of creating a new `Descriptor Wallet` each time and deletes
+CCoinControl GetNewCoinControl(FuzzedDataProvi
...