💬 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!
(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
(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?
(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
...
(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.
(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
(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?).
(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
...
(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.
(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
(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
(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
...
(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
...
💬 luke-jr commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1668797148)
>non-witness node
This isn't a thing. Witness data is no more prunable than other data, to a full node.
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1668797148)
>non-witness node
This isn't a thing. Witness data is no more prunable than other data, to a full node.
💬 elocremarc commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1668931827)
I hate to break it to you all. Bitcoin is primary a data layer and secondary a monetary layer. The data of transactions is worth spending to bribe miners for inclusion. However if some arbitrary data you disagree with gets inclusion and attestation and still follows the rules of consensus its a valid transaction. To censor any transaction that follows consensus is censorship.
Bitcoin is about uncensorable speech be it a monetary transaction or arbitrary data for someone willing to spend for
...
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1668931827)
I hate to break it to you all. Bitcoin is primary a data layer and secondary a monetary layer. The data of transactions is worth spending to bribe miners for inclusion. However if some arbitrary data you disagree with gets inclusion and attestation and still follows the rules of consensus its a valid transaction. To censor any transaction that follows consensus is censorship.
Bitcoin is about uncensorable speech be it a monetary transaction or arbitrary data for someone willing to spend for
...
💬 MarcoFalke commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1668952952)
Guix is shipping rust, according to https://packages.guix.gnu.org/packages/rust-cargo, so it can be bootstrapped on all systems that can be used to compile release binaries. I am not aware of any requirement nor efforts that the linters must be as bootstrappable as the release binaries, but at least the ones in this pull are, so that alone seems sufficient. On top of that, NixOS ships rust: https://github.com/NixOS/nixpkgs/tree/nixos-23.05/pkgs/development/compilers/rust, there is also mrustc fo
...
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1668952952)
Guix is shipping rust, according to https://packages.guix.gnu.org/packages/rust-cargo, so it can be bootstrapped on all systems that can be used to compile release binaries. I am not aware of any requirement nor efforts that the linters must be as bootstrappable as the release binaries, but at least the ones in this pull are, so that alone seems sufficient. On top of that, NixOS ships rust: https://github.com/NixOS/nixpkgs/tree/nixos-23.05/pkgs/development/compilers/rust, there is also mrustc fo
...
💬 MarcoFalke commented on pull request "net_processing: ensure mapBlockSource is removed from in ProcessBlock":
(https://github.com/bitcoin/bitcoin/pull/28235#issuecomment-1668954581)
Looks like the tests time all out after 2 hours? Also, could use `p2p` instead of `net_processing` as the prefix, according to the docs?
(https://github.com/bitcoin/bitcoin/pull/28235#issuecomment-1668954581)
Looks like the tests time all out after 2 hours? Also, could use `p2p` instead of `net_processing` as the prefix, according to the docs?
💬 Psifour commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1668958690)
Bitcoin is purely a layer for storage of data that, typically, represents financial transactions. We have witnessed for years that there is no reasonable way to prevent the storage of abitrary data (any system that allows user input of 'random' values should accept the inevitability of data storage, "the signal is the noise").
The internet became the ubiquitous system that it is today not because some weak-willed soul decided for the users that they needed to be protected by limiting the powe
...
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1668958690)
Bitcoin is purely a layer for storage of data that, typically, represents financial transactions. We have witnessed for years that there is no reasonable way to prevent the storage of abitrary data (any system that allows user input of 'random' values should accept the inevitability of data storage, "the signal is the noise").
The internet became the ubiquitous system that it is today not because some weak-willed soul decided for the users that they needed to be protected by limiting the powe
...
💬 MarcoFalke commented on pull request "refactor: Remove unused includes from wallet.cpp":
(https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1668972547)
@RandyMcMillan That looks unrelated to the changes here. You'll have to use `make clean && make distclean`, or something similar
(https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1668972547)
@RandyMcMillan That looks unrelated to the changes here. You'll have to use `make clean && make distclean`, or something similar
💬 MarcoFalke commented on pull request "bench: add readblock benchmark":
(https://github.com/bitcoin/bitcoin/pull/26684#discussion_r1286645023)
(Personally, I think this is a style nit and either version is fine here that compiles and has CI pass. Going forward, we should either remove the section from the docs (my preference), or enforce it with clang-tidy or something else in CI)
(https://github.com/bitcoin/bitcoin/pull/26684#discussion_r1286645023)
(Personally, I think this is a style nit and either version is fine here that compiles and has CI pass. Going forward, we should either remove the section from the docs (my preference), or enforce it with clang-tidy or something else in CI)
💬 kravets commented on pull request "wallet: Deniability API (Unilateral Transaction Meta-Privacy)":
(https://github.com/bitcoin/bitcoin/pull/27792#issuecomment-1668987567)
In a tx graph ( tree really with large and randomized fan out, eventual merging of some UTXOs does not necessarily indicate that the later clustered entity is the same entity KYC’s on an exchange prior to the deniabilization fan out
(https://github.com/bitcoin/bitcoin/pull/27792#issuecomment-1668987567)
In a tx graph ( tree really with large and randomized fan out, eventual merging of some UTXOs does not necessarily indicate that the later clustered entity is the same entity KYC’s on an exchange prior to the deniabilization fan out