💬 brunoerg commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1213295906)
In 31dd230738c43a529c63331eb23135e42d083e8f: At this point (creation of `open_sensitive_relay`), is there any possibility of both tor and i2p to be unreachable?
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1213295906)
In 31dd230738c43a529c63331eb23135e42d083e8f: At this point (creation of `open_sensitive_relay`), is there any possibility of both tor and i2p to be unreachable?
💬 vasild commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1213318442)
I agree with what you say above - `whitebind` on publicly accessible address:port with this new permission sounds bad. Similar with `whitelist` - if a range is used.
This PR currently expands the semantic of the `noban` permission. This will affect existent setups that already use it. Would it make sense to introduce a new permisson, separate from `noban`? I mean - now if somebody is running `-whitebind=noban@publicaddr:port` then a bad actor could cause harm on `master`, but even more harm w
...
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1213318442)
I agree with what you say above - `whitebind` on publicly accessible address:port with this new permission sounds bad. Similar with `whitelist` - if a range is used.
This PR currently expands the semantic of the `noban` permission. This will affect existent setups that already use it. Would it make sense to introduce a new permisson, separate from `noban`? I mean - now if somebody is running `-whitebind=noban@publicaddr:port` then a bad actor could cause harm on `master`, but even more harm w
...
💬 furszy commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#discussion_r1213346408)
Ended up not adding this change to not modify `AbortShutdown()`.
(https://github.com/bitcoin/bitcoin/pull/27708#discussion_r1213346408)
Ended up not adding this change to not modify `AbortShutdown()`.
💬 furszy commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#issuecomment-1572294451)
Feedback tackled. Thanks TheCharlatan!. Changes:
* Moved ThreadImport ABC error to use `AbortNode`. Per [comment](https://github.com/bitcoin/bitcoin/pull/27708#discussion_r1206967261).
* And moved `AbortNode` code below `StartShutdown` so it can use the introduced static function instead of having to add a public `StartErrorShutdown` function that is only used by the .cpp file and not externally.
(https://github.com/bitcoin/bitcoin/pull/27708#issuecomment-1572294451)
Feedback tackled. Thanks TheCharlatan!. Changes:
* Moved ThreadImport ABC error to use `AbortNode`. Per [comment](https://github.com/bitcoin/bitcoin/pull/27708#discussion_r1206967261).
* And moved `AbortNode` code below `StartShutdown` so it can use the introduced static function instead of having to add a public `StartErrorShutdown` function that is only used by the .cpp file and not externally.
💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1213371595)
Good observation. If both Tor and I2P are unreachable then `m_sensitive_relay_connections_to_open` should always be zero. Inside `PeerManagerImpl::ScheduleLocalTxForRelay()` `UseSensitiveRelay()` would return `false` and `m_connman.ScheduleSensitiveRelayConnections()` which increments the counter will never be called.
Now, that is the logic of some code far away, in another file. I did not want to rely on it because if we enter here and both are unreachable, then it will end up in an infinite
...
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1213371595)
Good observation. If both Tor and I2P are unreachable then `m_sensitive_relay_connections_to_open` should always be zero. Inside `PeerManagerImpl::ScheduleLocalTxForRelay()` `UseSensitiveRelay()` would return `false` and `m_connman.ScheduleSensitiveRelayConnections()` which increments the counter will never be called.
Now, that is the logic of some code far away, in another file. I did not want to rely on it because if we enter here and both are unreachable, then it will end up in an infinite
...
💬 ryanofsky commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1572348643)
Updated 5559ad2c69ef82c18843b9214e5ba3974834a1ae -> 2cd28e9fef5dd743bcd70025196ee311fcfdcae4 ([`pr/nonly.16`](https://github.com/ryanofsky/bitcoin/commits/pr/nonly.16) -> [`pr/nonly.17`](https://github.com/ryanofsky/bitcoin/commits/pr/nonly.17), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/nonly.16..pr/nonly.17)) with suggested test and conversion table fixes. I also changed the code to use actual position of the options parameters as originally suggested, instead of -1 values.
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1572348643)
Updated 5559ad2c69ef82c18843b9214e5ba3974834a1ae -> 2cd28e9fef5dd743bcd70025196ee311fcfdcae4 ([`pr/nonly.16`](https://github.com/ryanofsky/bitcoin/commits/pr/nonly.16) -> [`pr/nonly.17`](https://github.com/ryanofsky/bitcoin/commits/pr/nonly.17), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/nonly.16..pr/nonly.17)) with suggested test and conversion table fixes. I also changed the code to use actual position of the options parameters as originally suggested, instead of -1 values.
💬 vasild commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#issuecomment-1572354107)
Concept ACK, I guess we can forget about Torv2 and completely drop support about it from everywhere as if it never existed. The Tor network does not support those anymore.
(https://github.com/bitcoin/bitcoin/pull/27452#issuecomment-1572354107)
Concept ACK, I guess we can forget about Torv2 and completely drop support about it from everywhere as if it never existed. The Tor network does not support those anymore.
📝 ryanofsky opened a pull request: "wallet: Add tracing for sqlite statements"
(https://github.com/bitcoin/bitcoin/pull/27801)
I found sqlite tracing was useful for debugging a test in #27790, and thought it might be helpful in other contexts too, so this PR adds an option to enable it. Tracing is still disabled by default and only shown with `-debug=walletdb -loglevel=walletdb:trace` options.
(https://github.com/bitcoin/bitcoin/pull/27801)
I found sqlite tracing was useful for debugging a test in #27790, and thought it might be helpful in other contexts too, so this PR adds an option to enable it. Tracing is still disabled by default and only shown with `-debug=walletdb -loglevel=walletdb:trace` options.
👋 ryanofsky's pull request is ready for review: "streams: Drop confusing DataStream::Serialize method and << operator"
(https://github.com/bitcoin/bitcoin/pull/27800)
(https://github.com/bitcoin/bitcoin/pull/27800)
💬 vasild commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1213400231)
This could collide with another port randomly assigned by the testing framework (even if it looks like that it currently cannot, it may do in the future) and also could collide with another instance of this test run in parallel. Better use `p2p_port()`.
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1213400231)
This could collide with another port randomly assigned by the testing framework (even if it looks like that it currently cannot, it may do in the future) and also could collide with another instance of this test run in parallel. Better use `p2p_port()`.
💬 vasild commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1213410047)
Why is it necessary to modify the services?
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1213410047)
Why is it necessary to modify the services?
💬 ryanofsky commented on pull request "walletdb: Add PrefixCursor":
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1213432081)
Oops, I guess I was objecting to a point Marco wasn't making. But I think I would still like to avoid overloading Serialize for std::byte spans. The whole point of std::byte is that it's supposed to be a very safe type which requires you to be deliberate and explicit about conversions. So I'm not sure it would be great to serialize std::byte spans (or types which can be converted to byte spans) as raw bytes without requiring a more explicit cast.
Maybe it would make sense to change serialize.
...
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1213432081)
Oops, I guess I was objecting to a point Marco wasn't making. But I think I would still like to avoid overloading Serialize for std::byte spans. The whole point of std::byte is that it's supposed to be a very safe type which requires you to be deliberate and explicit about conversions. So I'm not sure it would be great to serialize std::byte spans (or types which can be converted to byte spans) as raw bytes without requiring a more explicit cast.
Maybe it would make sense to change serialize.
...
💬 glozow commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1213443871)
Not sure if we should load old (i.e. potentially bad) data as it can skew the fee estimates both at startup and after more data has been collected. For testing, they could mock the time right? Just worried about adding a config option that's a bit footgunny.
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1213443871)
Not sure if we should load old (i.e. potentially bad) data as it can skew the fee estimates both at startup and after more data has been collected. For testing, they could mock the time right? Just worried about adding a config option that's a bit footgunny.
💬 achow101 commented on pull request "walletdb: Add PrefixCursor":
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1213449316)
Good point, done as suggested.
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1213449316)
Good point, done as suggested.
💬 fanquake commented on pull request "build: produce a .zip for macOS distribution":
(https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1572459208)
> Guix does not produce unsigned
Fixed.
(https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1572459208)
> Guix does not produce unsigned
Fixed.
💬 ryanofsky commented on pull request "kernel: Remove shutdown from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1572503569)
I think I agree with Cory, and think it is a problem even if it is not a bug for `BlockManager::FlushBlockFile` and `BlockManager::FlushUndoFile` methods to call `Notifications::fatalError` without returning errors themselves. The fatal errors there seem different than other fatal errors, and the contradict the "whatever function triggered the error should also return an error code or raise an exception" documention.
I would suggest making one (or both) of the following changes to this PR:
...
(https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1572503569)
I think I agree with Cory, and think it is a problem even if it is not a bug for `BlockManager::FlushBlockFile` and `BlockManager::FlushUndoFile` methods to call `Notifications::fatalError` without returning errors themselves. The fatal errors there seem different than other fatal errors, and the contradict the "whatever function triggered the error should also return an error code or raise an exception" documention.
I would suggest making one (or both) of the following changes to this PR:
...
💬 instagibbs commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#issuecomment-1572526838)
> redownloading orphans if we cannot afford to protect them
Is the idea here that protection is a bandiwdth optimization to avoid fetching an orphan twice in a row? Reasonable if so, just want this to be explicitly stated if so.
(https://github.com/bitcoin/bitcoin/pull/27742#issuecomment-1572526838)
> redownloading orphans if we cannot afford to protect them
Is the idea here that protection is a bandiwdth optimization to avoid fetching an orphan twice in a row? Reasonable if so, just want this to be explicitly stated if so.
🤔 Xekyo reviewed a pull request: "fuzz: improve `coinselection`"
(https://github.com/bitcoin/bitcoin/pull/27585#pullrequestreview-1455990639)
I’m pretty new to fuzz testing, so I am not sure whether we want to have all these additional calls for each selection result. It seems to me that if we wanted to test the behavior of SelectionResult, it might make more sense to separately fuzz that instead of fuzzing that in the context of running all the different coin selection algorithms, since it might lower the number of executions we get on fuzzing coin selection.
OTOH, it’s great to get fuzzing coverage for these codepaths.
Code ch
...
(https://github.com/bitcoin/bitcoin/pull/27585#pullrequestreview-1455990639)
I’m pretty new to fuzz testing, so I am not sure whether we want to have all these additional calls for each selection result. It seems to me that if we wanted to test the behavior of SelectionResult, it might make more sense to separately fuzz that instead of fuzzing that in the context of running all the different coin selection algorithms, since it might lower the number of executions we get on fuzzing coin selection.
OTOH, it’s great to get fuzzing coverage for these codepaths.
Code ch
...
💬 Xekyo commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1213479178)
Let’s allow the lower bound of what’s possible:
```suggestion
const int n_input_bytes{fuzzed_data_provider.ConsumeIntegralInRange<int>(41, 10000)};
```
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1213479178)
Let’s allow the lower bound of what’s possible:
```suggestion
const int n_input_bytes{fuzzed_data_provider.ConsumeIntegralInRange<int>(41, 10000)};
```
💬 Xekyo commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1213485109)
It’s not clear to me why the `ComputeAndSetWaste()` is called with these parameters. The first should be `min_viable_change` which should be bigger than `change_fee` but not really related to `cost_of_change`. I guess that’s fine for this test. It’s also not clear to me, though why the `change_fee` is set to 0 here. Wouldn’t it be better to have it derive from the current `coin_params`?
`coin_params.m_change_fee = coin_params.m_effective_feerate.GetFee(coin_params.change_output_size);`
Not
...
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1213485109)
It’s not clear to me why the `ComputeAndSetWaste()` is called with these parameters. The first should be `min_viable_change` which should be bigger than `change_fee` but not really related to `cost_of_change`. I guess that’s fine for this test. It’s also not clear to me, though why the `change_fee` is set to 0 here. Wouldn’t it be better to have it derive from the current `coin_params`?
`coin_params.m_change_fee = coin_params.m_effective_feerate.GetFee(coin_params.change_output_size);`
Not
...