💬 mzumsande commented on issue "net: Tor service target port collides when running multiple nodes, making bitcoind error out":
(https://github.com/bitcoin/bitcoin/issues/31133#issuecomment-2465734085)
> (yes, I think it is a subset of bind=, somebody correct me if I am wrong)
As far as I understand it, one difference is that `-bind` disables self-discovery of addresses because `connOptions.bind_on_any` will not be set - whereas `-port` will work together with bind-on-any. So if you want to use `Discover()` together with a non-default port, `-port` is the only option.
(https://github.com/bitcoin/bitcoin/issues/31133#issuecomment-2465734085)
> (yes, I think it is a subset of bind=, somebody correct me if I am wrong)
As far as I understand it, one difference is that `-bind` disables self-discovery of addresses because `connOptions.bind_on_any` will not be set - whereas `-port` will work together with bind-on-any. So if you want to use `Discover()` together with a non-default port, `-port` is the only option.
👍 darosior approved a pull request: "descriptor: Add proper Clone function to miniscript::Node"
(https://github.com/bitcoin/bitcoin/pull/30866#pullrequestreview-2424940909)
ACK 6a00ea17c136e67a66f3558dd2d02a07860b0afe
(https://github.com/bitcoin/bitcoin/pull/30866#pullrequestreview-2424940909)
ACK 6a00ea17c136e67a66f3558dd2d02a07860b0afe
💬 darosior commented on issue "Listen on random port by default (not 8333)":
(https://github.com/bitcoin/bitcoin/issues/31036#issuecomment-2465795279)
> What about having P2P-seeds, an alternative to DNS-seeds
I guess the biggest drawback with this is how bootstrapping nodes will actually connect to this "P2P-seeds" instead of leveraging the DNS cache of their ISP.
(https://github.com/bitcoin/bitcoin/issues/31036#issuecomment-2465795279)
> What about having P2P-seeds, an alternative to DNS-seeds
I guess the biggest drawback with this is how bootstrapping nodes will actually connect to this "P2P-seeds" instead of leveraging the DNS cache of their ISP.
💬 darosior commented on issue "Listen on random port by default (not 8333)":
(https://github.com/bitcoin/bitcoin/issues/31036#issuecomment-2465821538)
Besides the load, which can be dealt with by having a large number of seeds, i wonder how much getting rid of the DNS caching impacts "privacy", loosely defined. Of course from the perspective of a node, if you want to hide your IP you use an anonymizing network period. But it's also the case that being a P2P seed gives you a privileged observation position on all the nodes joining the network, which is less true for DNS seeds since most of the time the bootstrapping nodes would not actually con
...
(https://github.com/bitcoin/bitcoin/issues/31036#issuecomment-2465821538)
Besides the load, which can be dealt with by having a large number of seeds, i wonder how much getting rid of the DNS caching impacts "privacy", loosely defined. Of course from the perspective of a node, if you want to hide your IP you use an anonymizing network period. But it's also the case that being a P2P seed gives you a privileged observation position on all the nodes joining the network, which is less true for DNS seeds since most of the time the bootstrapping nodes would not actually con
...
🤔 hodlinator reviewed a pull request: "Ephemeral Dust"
(https://github.com/bitcoin/bitcoin/pull/30239#pullrequestreview-2420454069)
Concept ACK 131bed19bdfc922328cad9781fa9122b6810a8fd
Thanks for working on this! Seems like an obvious win for L2s.
Haven't yet absorbed the full context sufficiently to fully A-C-K it myself. Hopefully my comments here and above provide value even though most are surface level. My personal priority with this review is to get it in better shape for others so they can A-C-K more easily if they agree.
(https://github.com/bitcoin/bitcoin/pull/30239#pullrequestreview-2420454069)
Concept ACK 131bed19bdfc922328cad9781fa9122b6810a8fd
Thanks for working on this! Seems like an obvious win for L2s.
Haven't yet absorbed the full context sufficiently to fully A-C-K it myself. Hopefully my comments here and above provide value even though most are surface level. My personal priority with this review is to get it in better shape for others so they can A-C-K more easily if they agree.
💬 hodlinator commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1832718055)
```suggestion
// so the last transaction to be generated (in a >1 package) must spend all package-made outputs
```
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1832718055)
```suggestion
// so the last transaction to be generated (in a >1 package) must spend all package-made outputs
```
💬 hodlinator commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1832702717)
Only exercising v2 - maybe you could describe the reasoning behind the test at the top of fuzz target?
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1832702717)
Only exercising v2 - maybe you could describe the reasoning behind the test at the top of fuzz target?
💬 hodlinator commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1832453654)
(Just curious - this is a very common pattern, why do we leave it up to the fuzz engine instead of just doing:
```suggestion
LIMITED_WHILE(true, 300)
```
Does it help define discrete sections of the fuzz data, which ends up being useful somehow?
These kind of uses make more sense to me: `LIMITED_WHILE(provider.remaining_bytes() > 0, iter_limit)`, `LIMITED_WHILE(!available_coins.empty(), 500)`.)
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1832453654)
(Just curious - this is a very common pattern, why do we leave it up to the fuzz engine instead of just doing:
```suggestion
LIMITED_WHILE(true, 300)
```
Does it help define discrete sections of the fuzz data, which ends up being useful somehow?
These kind of uses make more sense to me: `LIMITED_WHILE(provider.remaining_bytes() > 0, iter_limit)`, `LIMITED_WHILE(!available_coins.empty(), 500)`.)
💬 hodlinator commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1832376920)
"Also"? But in this case the sweep-transaction survives the reorg, unlike the case above.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1832376920)
"Also"? But in this case the sweep-transaction survives the reorg, unlike the case above.
💬 hodlinator commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1832400954)
Why 25 and not 1 block?
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1832400954)
Why 25 and not 1 block?
💬 hodlinator commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1832492686)
nit: Reduce the number of variables:
```suggestion
std::optional<COutPoint> outpoint_to_rbf{fuzzed_data_provider.ConsumeBool() ? GetChildEvictingPrevout(tx_pool) : std::nullopt};
```
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1832492686)
nit: Reduce the number of variables:
```suggestion
std::optional<COutPoint> outpoint_to_rbf{fuzzed_data_provider.ConsumeBool() ? GetChildEvictingPrevout(tx_pool) : std::nullopt};
```
💬 hodlinator commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1832985488)
Instead of creating two variables named `tx` here and on line 288, `emplace_back` the return value of the lambda:
```suggestion
txs.push_back([&] {
```
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1832985488)
Instead of creating two variables named `tx` here and on line 288, `emplace_back` the return value of the lambda:
```suggestion
txs.push_back([&] {
```
💬 hodlinator commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1832788903)
nit: Declare both `const` to be consistent between `num_in` & `num_out`
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1832788903)
nit: Declare both `const` to be consistent between `num_in` & `num_out`
💬 hodlinator commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1834500036)
```suggestion
// Pop random outpoint. We erase them to avoid double-spending
// while in this loop, but later add them back (unless last_tx).
```
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1834500036)
```suggestion
// Pop random outpoint. We erase them to avoid double-spending
// while in this loop, but later add them back (unless last_tx).
```
👍 tdb3 approved a pull request: "test: clarify log messages when handling SOCKS5 proxy connections"
(https://github.com/bitcoin/bitcoin/pull/31239#pullrequestreview-2425008603)
code review ACK 99d9a093cf6d53b24d4a48f5845e0e0299f47800
While we're updating this file, shouldn't docstring for `handle()` say `RFC1928` instead of `RFC192`? https://www.rfc-editor.org/rfc/rfc1928.txt
```python
def handle(self):
"""Handle socks5 request according to RFC192."""
```
(https://github.com/bitcoin/bitcoin/pull/31239#pullrequestreview-2425008603)
code review ACK 99d9a093cf6d53b24d4a48f5845e0e0299f47800
While we're updating this file, shouldn't docstring for `handle()` say `RFC1928` instead of `RFC192`? https://www.rfc-editor.org/rfc/rfc1928.txt
```python
def handle(self):
"""Handle socks5 request according to RFC192."""
```
💬 furszy commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1835111673)
In f3b0593b8:
Can you explain why did you remove the other coins (2 and 4) that were here before? Guess they will be selected and break something if we re-add them because of the algo predilection for unifying utxos?
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1835111673)
In f3b0593b8:
Can you explain why did you remove the other coins (2 and 4) that were here before? Guess they will be selected and break something if we re-add them because of the algo predilection for unifying utxos?
💬 furszy commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1835113240)
In https://github.com/bitcoin/bitcoin/commit/f3b0593b8bd67bc919b21cd636a01813fc5538c8:
This could output a bit more detailed error message:
```c++
BOOST_CHECK_MESSAGE(EquivalentResult(expected_result, *result), strprintf("Result mismatch in BnB-Success: %s. Expected %s vs %s", test_title, InputsToString(expected_result), InputsToString(*result)));
BOOST_CHECK_MESSAGE(result->GetSelectedValue() == expected_amount, strprintf("Selected amount mismatch in BnB-Success: %s. Expected %d vs %d", t
...
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1835113240)
In https://github.com/bitcoin/bitcoin/commit/f3b0593b8bd67bc919b21cd636a01813fc5538c8:
This could output a bit more detailed error message:
```c++
BOOST_CHECK_MESSAGE(EquivalentResult(expected_result, *result), strprintf("Result mismatch in BnB-Success: %s. Expected %s vs %s", test_title, InputsToString(expected_result), InputsToString(*result)));
BOOST_CHECK_MESSAGE(result->GetSelectedValue() == expected_amount, strprintf("Selected amount mismatch in BnB-Success: %s. Expected %d vs %d", t
...
📝 secp512k2 opened a pull request: "doc: Fix missing comma in JSON example in REST-interface.md"
(https://github.com/bitcoin/bitcoin/pull/31259)
This pull request addresses a minor issues in the REST-interface.md documentation:
Missing Comma in JSON Example: In the "Query UTXO set" section, a missing comma after the "desc" field in the JSON example has been added to ensure valid JSON syntax.
(https://github.com/bitcoin/bitcoin/pull/31259)
This pull request addresses a minor issues in the REST-interface.md documentation:
Missing Comma in JSON Example: In the "Query UTXO set" section, a missing comma after the "desc" field in the JSON example has been added to ensure valid JSON syntax.
📝 ryanofsky opened a pull request: "WIP: scripted-diff: Type-safe settings retrieval"
(https://github.com/bitcoin/bitcoin/pull/31260)
This PR changes the way settings are registered and retrieved to provide more compile-time safety. Currently settings are [registered](https://github.com/ryanofsky/bitcoin/blob/03cff2c1421e5db59963eba1a845ef5dd318c275/src/init.cpp#L504) like:
```c++
argsman.AddArg("-pid=<file>", strprintf("Specify pid file. Relative paths will be prefixed by a net-specific datadir location. (default: %s)", BITCOIN_PID_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
```
and [retrieved](https
...
(https://github.com/bitcoin/bitcoin/pull/31260)
This PR changes the way settings are registered and retrieved to provide more compile-time safety. Currently settings are [registered](https://github.com/ryanofsky/bitcoin/blob/03cff2c1421e5db59963eba1a845ef5dd318c275/src/init.cpp#L504) like:
```c++
argsman.AddArg("-pid=<file>", strprintf("Specify pid file. Relative paths will be prefixed by a net-specific datadir location. (default: %s)", BITCOIN_PID_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
```
and [retrieved](https
...
📝 ryanofsky converted_to_draft a pull request: "WIP: scripted-diff: Type-safe settings retrieval"
(https://github.com/bitcoin/bitcoin/pull/31260)
This PR changes the way settings are registered and retrieved to provide more compile-time safety. Currently settings are [registered](https://github.com/ryanofsky/bitcoin/blob/03cff2c1421e5db59963eba1a845ef5dd318c275/src/init.cpp#L504) like:
```c++
argsman.AddArg("-pid=<file>", strprintf("Specify pid file. Relative paths will be prefixed by a net-specific datadir location. (default: %s)", BITCOIN_PID_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
```
and [retrieved](https
...
(https://github.com/bitcoin/bitcoin/pull/31260)
This PR changes the way settings are registered and retrieved to provide more compile-time safety. Currently settings are [registered](https://github.com/ryanofsky/bitcoin/blob/03cff2c1421e5db59963eba1a845ef5dd318c275/src/init.cpp#L504) like:
```c++
argsman.AddArg("-pid=<file>", strprintf("Specify pid file. Relative paths will be prefixed by a net-specific datadir location. (default: %s)", BITCOIN_PID_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
```
and [retrieved](https
...