💬 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
💬 ariard commented on issue "Proposal for a new mempool design":
(https://github.com/bitcoin/bitcoin/issues/27677#issuecomment-1572647968)
From my understanding of the proposal, there is an intuition of aligning mining (i.e the maximum fees in a block template)
and transactions evicted from our local mempools. The proposed changes goals would be to make the transactions that would be the last ones selected by our mining algorithm, the first ones to be removed from the mempool based e.g on an ancestor-feerate-based mining algorithm.
While this symmetrization of block template construction and mempool eviction should work in ligh
...
(https://github.com/bitcoin/bitcoin/issues/27677#issuecomment-1572647968)
From my understanding of the proposal, there is an intuition of aligning mining (i.e the maximum fees in a block template)
and transactions evicted from our local mempools. The proposed changes goals would be to make the transactions that would be the last ones selected by our mining algorithm, the first ones to be removed from the mempool based e.g on an ancestor-feerate-based mining algorithm.
While this symmetrization of block template construction and mempool eviction should work in ligh
...
💬 theuni commented on pull request "kernel: Remove shutdown from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1572651644)
> Changing `FlushBlockFile` and `FlushUndoFile` to return `[[nodiscard]] bool`, and changing calls to these functions either bubble up the error or have comments about why the error is safe to ignore in that particular context.
I learned while working on the bubble-up stuff that a _type_ can be `[[nodiscard]]` too. [You can see an example of it here ](https://github.com/theuni/bitcoin/blob/clang-tidy-shutdown-rebase/src/early_exit.h#L45)to force bubble-up warnings.
Rather than returnin
...
(https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1572651644)
> Changing `FlushBlockFile` and `FlushUndoFile` to return `[[nodiscard]] bool`, and changing calls to these functions either bubble up the error or have comments about why the error is safe to ignore in that particular context.
I learned while working on the bubble-up stuff that a _type_ can be `[[nodiscard]]` too. [You can see an example of it here ](https://github.com/theuni/bitcoin/blob/clang-tidy-shutdown-rebase/src/early_exit.h#L45)to force bubble-up warnings.
Rather than returnin
...
💬 MarcoFalke commented on pull request "Update .style.yapf":
(https://github.com/bitcoin/bitcoin/pull/27802#issuecomment-1572654966)
lgtm ACK bc70072de1dd7d82d0ab95a10b507af94078065c
(https://github.com/bitcoin/bitcoin/pull/27802#issuecomment-1572654966)
lgtm ACK bc70072de1dd7d82d0ab95a10b507af94078065c
🚀 achow101 merged a pull request: "RPC: Accept options as named-only parameters"
(https://github.com/bitcoin/bitcoin/pull/26485)
(https://github.com/bitcoin/bitcoin/pull/26485)
👍 stickies-v approved a pull request: "[25.x] build: disable boost multi index safe mode"
(https://github.com/bitcoin/bitcoin/pull/27775#pullrequestreview-1456204243)
ACK 9dc584849
Identical to the changes in 59c894474 (#27724), except two changes ([1](https://github.com/bitcoin/bitcoin/pull/27662/files#diff-f0dc3382c957aaa68adb93e6cd910b213e3982110f50182c3d806b43912952baR14-R15), [2](https://github.com/bitcoin/bitcoin/pull/27483/files#diff-f2f2004cbc4381f921708619aa9255979a5b95d49d3739c20f6a4a7490c3d58fR20-R21)) to the CI setup which I think can safely be ignored.
(https://github.com/bitcoin/bitcoin/pull/27775#pullrequestreview-1456204243)
ACK 9dc584849
Identical to the changes in 59c894474 (#27724), except two changes ([1](https://github.com/bitcoin/bitcoin/pull/27662/files#diff-f0dc3382c957aaa68adb93e6cd910b213e3982110f50182c3d806b43912952baR14-R15), [2](https://github.com/bitcoin/bitcoin/pull/27483/files#diff-f2f2004cbc4381f921708619aa9255979a5b95d49d3739c20f6a4a7490c3d58fR20-R21)) to the CI setup which I think can safely be ignored.