📝 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
💬 sdaftuar commented on issue "Proposal for a new mempool design":
(https://github.com/bitcoin/bitcoin/issues/27677#issuecomment-1572728184)
> Namely, high-feerate evictated/replaced chunks of transactions could be kept in a buffer of transactions ordered by their last descendants outputs. When a new package comes in, in case of missing parents outputs, a lookup is realized in the buffer to see if the package attached to evicted/replaced chunks ancestor-feerate is above the cluster spending the same utxo or above the bottom mempool chunks.
[snip additional comments related to this]
If I'm understanding your post correctly, you'
...
(https://github.com/bitcoin/bitcoin/issues/27677#issuecomment-1572728184)
> Namely, high-feerate evictated/replaced chunks of transactions could be kept in a buffer of transactions ordered by their last descendants outputs. When a new package comes in, in case of missing parents outputs, a lookup is realized in the buffer to see if the package attached to evicted/replaced chunks ancestor-feerate is above the cluster spending the same utxo or above the bottom mempool chunks.
[snip additional comments related to this]
If I'm understanding your post correctly, you'
...
👋 achow101's pull request is ready for review: "rpc: Be able to access RPC parameters by name"
(https://github.com/bitcoin/bitcoin/pull/27788)
(https://github.com/bitcoin/bitcoin/pull/27788)
💬 brunoerg commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1213654672)
Great, done!
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1213654672)
Great, done!
💬 hebasto commented on pull request "guix: fix CURL disable flag in osslsigncode":
(https://github.com/bitcoin/bitcoin/pull/27779#issuecomment-1572754794)
> It is not clear why this is a fix. `CMAKE_DISABLE_FIND_PACKAGE_CURL` is a valid [variable](https://cmake.org/cmake/help/latest/variable/CMAKE_DISABLE_FIND_PACKAGE_PackageName.html) that:
>
> > can be used to build a project without an optional package
Considering the `FindCURL` module [source code](https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/FindCURL.cmake), I believe that the current using of the `CMAKE_DISABLE_FIND_PACKAGE_CURL` variable is optimal.
FWIW, the build
...
(https://github.com/bitcoin/bitcoin/pull/27779#issuecomment-1572754794)
> It is not clear why this is a fix. `CMAKE_DISABLE_FIND_PACKAGE_CURL` is a valid [variable](https://cmake.org/cmake/help/latest/variable/CMAKE_DISABLE_FIND_PACKAGE_PackageName.html) that:
>
> > can be used to build a project without an optional package
Considering the `FindCURL` module [source code](https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/FindCURL.cmake), I believe that the current using of the `CMAKE_DISABLE_FIND_PACKAGE_CURL` variable is optimal.
FWIW, the build
...
💬 brunoerg commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1213672054)
I didn't touch `ComputeAndSetWaste` parameters value, perhaps who did it copied (?) from: https://github.com/bitcoin/bitcoin/blob/34ac3f438a268e83af6cd11e2981e5bc07f699c9/src/wallet/coinselection.cpp#L179
I'm going to change it to make it more "realistic"/robust.
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1213672054)
I didn't touch `ComputeAndSetWaste` parameters value, perhaps who did it copied (?) from: https://github.com/bitcoin/bitcoin/blob/34ac3f438a268e83af6cd11e2981e5bc07f699c9/src/wallet/coinselection.cpp#L179
I'm going to change it to make it more "realistic"/robust.
💬 kevkevinpal commented on pull request "wallet: Add tracing for sqlite statements":
(https://github.com/bitcoin/bitcoin/pull/27801#issuecomment-1572777861)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/27801#issuecomment-1572777861)
Concept ACK