💬 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
...
📝 theblackmace opened a pull request: "Update .style.yapf"
(https://github.com/bitcoin/bitcoin/pull/27802)
Corrected a minor typo
(https://github.com/bitcoin/bitcoin/pull/27802)
Corrected a minor typo
💬 ryanofsky commented on pull request "kernel: Remove shutdown from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27711#discussion_r1213477846)
In commit "kernel: Pass reference of global shutdown requested atomic" (978c05d58cea9f96f2713667663af35b43905701)
I don't think I understand what the purpose of this option is at a high level. If I am writing an application which uses libbitcoinkernel, I the thing I would want from the kernel library is a cancel() function I could call to cancel potentially slow operations. I don't want to have to mirror my application state to an `atomic<bool>` and be give the kernel read/write access to the
...
(https://github.com/bitcoin/bitcoin/pull/27711#discussion_r1213477846)
In commit "kernel: Pass reference of global shutdown requested atomic" (978c05d58cea9f96f2713667663af35b43905701)
I don't think I understand what the purpose of this option is at a high level. If I am writing an application which uses libbitcoinkernel, I the thing I would want from the kernel library is a cancel() function I could call to cancel potentially slow operations. I don't want to have to mirror my application state to an `atomic<bool>` and be give the kernel read/write access to the
...
💬 brunoerg commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1213534764)
I think there is no need of creating a global variable like `g_p2p_index`. We could do something like:
```diff
diff --git a/test/functional/p2p_local_tx_relay.py b/test/functional/p2p_local_tx_relay.py
index 0b115352d..9f4c6aaa5 100755
--- a/test/functional/p2p_local_tx_relay.py
+++ b/test/functional/p2p_local_tx_relay.py
@@ -36,12 +36,6 @@ MAX_OUTBOUND_FULL_RELAY_CONNECTIONS = 8
MAX_BLOCK_RELAY_ONLY_CONNECTIONS = 2
SENSITIVE_RELAY_NUM_BROADCAST_PER_TX = 5
-g_p2p_index = None
-def
...
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1213534764)
I think there is no need of creating a global variable like `g_p2p_index`. We could do something like:
```diff
diff --git a/test/functional/p2p_local_tx_relay.py b/test/functional/p2p_local_tx_relay.py
index 0b115352d..9f4c6aaa5 100755
--- a/test/functional/p2p_local_tx_relay.py
+++ b/test/functional/p2p_local_tx_relay.py
@@ -36,12 +36,6 @@ MAX_OUTBOUND_FULL_RELAY_CONNECTIONS = 8
MAX_BLOCK_RELAY_ONLY_CONNECTIONS = 2
SENSITIVE_RELAY_NUM_BROADCAST_PER_TX = 5
-g_p2p_index = None
-def
...
👍 ryanofsky approved a pull request: "walletdb: Add PrefixCursor"
(https://github.com/bitcoin/bitcoin/pull/27790#pullrequestreview-1456084608)
Code review ACK ba616b932cb9e9adb7eb9f1826caa62ce422a22d. Just suggested changes since last review
(https://github.com/bitcoin/bitcoin/pull/27790#pullrequestreview-1456084608)
Code review ACK ba616b932cb9e9adb7eb9f1826caa62ce422a22d. Just suggested changes since last review
💬 achow101 commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1572631032)
ACK 2cd28e9fef5dd743bcd70025196ee311fcfdcae4
Only changes since last are the suggested conversion table changes
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1572631032)
ACK 2cd28e9fef5dd743bcd70025196ee311fcfdcae4
Only changes since last are the suggested conversion table changes