💬 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.
💬 brunoerg commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1213614742)
I suppose it bypass the `maxconnection`, does it? If so, it worths to update the `-maxconnections` mentioning it.
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1213614742)
I suppose it bypass the `maxconnection`, does it? If so, it worths to update the `-maxconnections` mentioning it.
💬 Xekyo commented on issue "fuzz: mini_miner: Timeout in mini_miner":
(https://github.com/bitcoin/bitcoin/issues/27799#issuecomment-1572704415)
Yeah, it’s a grandfather paradox, in this fuzz seed, a transaction is it’s own great-grandparent. Looking into the fix.
(https://github.com/bitcoin/bitcoin/issues/27799#issuecomment-1572704415)
Yeah, it’s a grandfather paradox, in this fuzz seed, a transaction is it’s own great-grandparent. Looking into the fix.
💬 TheCharlatan commented on pull request "kernel: Remove shutdown from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27711#discussion_r1213631522)
Thank you for taking a look!
> So it seems to me the approach here is not very friendly to potential libbitcoinkernel applications, and is not even very friendly to our own application since it exposes the fRequestShutdown global more broadly and makes it harder to get rid of.
I was trying this out today on my proof of concept branch and got the same impression. It is unwieldy and the naming is bad.
> I think a better approach would be to get rid of the fRequestShutdown global and repla
...
(https://github.com/bitcoin/bitcoin/pull/27711#discussion_r1213631522)
Thank you for taking a look!
> So it seems to me the approach here is not very friendly to potential libbitcoinkernel applications, and is not even very friendly to our own application since it exposes the fRequestShutdown global more broadly and makes it harder to get rid of.
I was trying this out today on my proof of concept branch and got the same impression. It is unwieldy and the naming is bad.
> I think a better approach would be to get rid of the fRequestShutdown global and repla
...
💬 achow101 commented on pull request "Raise on invalid -debug and -loglevel config options":
(https://github.com/bitcoin/bitcoin/pull/27632#issuecomment-1572723855)
ACK 1a27bec1e9e40166d6ff7f0492115107289e7609
(https://github.com/bitcoin/bitcoin/pull/27632#issuecomment-1572723855)
ACK 1a27bec1e9e40166d6ff7f0492115107289e7609