🤔 murchandamus reviewed a pull request: "docs: add doc comment for SRD selection algorithm"
(https://github.com/bitcoin/bitcoin/pull/33622#pullrequestreview-3411003678)
Concept ACK on improving the documentation, but there is room for improvement on the specific phrasing.
(https://github.com/bitcoin/bitcoin/pull/33622#pullrequestreview-3411003678)
Concept ACK on improving the documentation, but there is room for improvement on the specific phrasing.
💬 murchandamus commented on pull request "docs: add doc comment for SRD selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/33622#discussion_r2486466028)
This comment is unhelpful. It provides the information that is already available from the code, but does not correctly explain the abstract context.
```suggestion
* @param[in] change_fee The cost of adding the change output to the transaction at the transaction’s feerate. Budgeting separately ensures that the change’s amount will be at least CHANGE_LOWER.
```
(https://github.com/bitcoin/bitcoin/pull/33622#discussion_r2486466028)
This comment is unhelpful. It provides the information that is already available from the code, but does not correctly explain the abstract context.
```suggestion
* @param[in] change_fee The cost of adding the change output to the transaction at the transaction’s feerate. Budgeting separately ensures that the change’s amount will be at least CHANGE_LOWER.
```
💬 murchandamus commented on pull request "docs: add doc comment for SRD selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/33622#discussion_r2486515535)
SRD does _NOT_ minimize the number of UTXOs included in the result. It only ensures that the selection adheres to the weight limit. *Minimizing* would imply that the selection is further pruned beyond adhering to the weight limit. The last sentence should also be amended to clarify that SRD does not always return a result, but when it does, the selection will always create a change output:
```suggestion
* outputs until the target is satisfied. If the maximum weight is exceeded, the OutputG
...
(https://github.com/bitcoin/bitcoin/pull/33622#discussion_r2486515535)
SRD does _NOT_ minimize the number of UTXOs included in the result. It only ensures that the selection adheres to the weight limit. *Minimizing* would imply that the selection is further pruned beyond adhering to the weight limit. The last sentence should also be amended to clarify that SRD does not always return a result, but when it does, the selection will always create a change output:
```suggestion
* outputs until the target is satisfied. If the maximum weight is exceeded, the OutputG
...
📝 purpleKarrot opened a pull request: "refactor: C++20 operators"
(https://github.com/bitcoin/bitcoin/pull/33771)
Remove all `operator!=` definitions and provide `operator<=>` as a replacement where all relational comparison operators were defined before.
The compiler is able to deduce missing comparison operators from `operator!=` and `operator<=>`. The compiler provided operators have the following advantages:
1. less code
2. guaranteed consistency
Refactoring that changes the implementation, or replaces it with `= default` is left for a separate PR.
(https://github.com/bitcoin/bitcoin/pull/33771)
Remove all `operator!=` definitions and provide `operator<=>` as a replacement where all relational comparison operators were defined before.
The compiler is able to deduce missing comparison operators from `operator!=` and `operator<=>`. The compiler provided operators have the following advantages:
1. less code
2. guaranteed consistency
Refactoring that changes the implementation, or replaces it with `= default` is left for a separate PR.
🤔 hodlinator reviewed a pull request: "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer"
(https://github.com/bitcoin/bitcoin/pull/32606#pullrequestreview-3411027758)
Reviewed 32bfd6136134e7194ea0b56ec08498f66829fc40
(https://github.com/bitcoin/bitcoin/pull/32606#pullrequestreview-3411027758)
Reviewed 32bfd6136134e7194ea0b56ec08498f66829fc40
💬 hodlinator commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2486484296)
This clearing should arguably happen before the assert, along the lines of https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2466726988 / 32bfd6136134e7194ea0b56ec08498f66829fc40
<details><summary>Full diff, including some previously missing clear_getblocktxn(), and deferring clearing to the last possible place.</summary>
```diff
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -414,6 +414,7 @@ class CompactBlocksTest(BitcoinTestFramework)
...
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2486484296)
This clearing should arguably happen before the assert, along the lines of https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2466726988 / 32bfd6136134e7194ea0b56ec08498f66829fc40
<details><summary>Full diff, including some previously missing clear_getblocktxn(), and deferring clearing to the last possible place.</summary>
```diff
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -414,6 +414,7 @@ class CompactBlocksTest(BitcoinTestFramework)
...
💬 hodlinator commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2486510648)
nit: Named variable feels clearer. (No risk of confusing with the index used for `block.vtx[...]`).
```suggestion
too_high_index = 1
prefilled_txn = PrefilledTransaction(too_high_index, block.vtx[0])
```
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2486510648)
nit: Named variable feels clearer. (No risk of confusing with the index used for `block.vtx[...]`).
```suggestion
too_high_index = 1
prefilled_txn = PrefilledTransaction(too_high_index, block.vtx[0])
```
💬 hodlinator commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2486492739)
Thanks for using separate peers in 42e281dba0f0807cc08d8c50a9d19906e27e2129. Was totally not expecting the reassignment of `segwit_node`. :sleeping:
I like how you use `hb_peer_idx` in f24237ef3d8c94c01f4b9f86a7d28d0b5fcdc75b as both a parameter for `assert_highbandwidth_states()` and for indexing into `self.nodes[0].p2ps`.
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2486492739)
Thanks for using separate peers in 42e281dba0f0807cc08d8c50a9d19906e27e2129. Was totally not expecting the reassignment of `segwit_node`. :sleeping:
I like how you use `hb_peer_idx` in f24237ef3d8c94c01f4b9f86a7d28d0b5fcdc75b as both a parameter for `assert_highbandwidth_states()` and for indexing into `self.nodes[0].p2ps`.
💬 average-gary commented on pull request "test: Enhance GetTxSigOpCost tests for coinbase transactions":
(https://github.com/bitcoin/bitcoin/pull/32840#discussion_r2486611787)
It seemed lacking since it was only tested for one script type (p2wpkh).
(https://github.com/bitcoin/bitcoin/pull/32840#discussion_r2486611787)
It seemed lacking since it was only tested for one script type (p2wpkh).
👍 willcl-ark approved a pull request: "ci: gha: Set debug_pull_request_number_str annotation"
(https://github.com/bitcoin/bitcoin/pull/33754#pullrequestreview-3411229739)
ACK fa9d0f994b45a94e3f26c01e395c58ff59f47f43
Only glanced at the Drahtbot change, but this makes sense as a way to get the PR number out for it.
(https://github.com/bitcoin/bitcoin/pull/33754#pullrequestreview-3411229739)
ACK fa9d0f994b45a94e3f26c01e395c58ff59f47f43
Only glanced at the Drahtbot change, but this makes sense as a way to get the PR number out for it.
👍 rkrux approved a pull request: "doc: corrected lockunspent rpc quoting"
(https://github.com/bitcoin/bitcoin/pull/31275#pullrequestreview-3411248713)
crACK 7e93e2925981c78c17a3deb2f6e6a16fa56730c4
Can update the PR title now that more RPCs are updated.
(https://github.com/bitcoin/bitcoin/pull/31275#pullrequestreview-3411248713)
crACK 7e93e2925981c78c17a3deb2f6e6a16fa56730c4
Can update the PR title now that more RPCs are updated.
👋 yuvicc's pull request is ready for review: "rpc, test: allow multiple data outputs in `createrawtransaction` & `createpsbt`"
(https://github.com/bitcoin/bitcoin/pull/32790)
(https://github.com/bitcoin/bitcoin/pull/32790)
🚀 fanquake merged a pull request: "ci: gha: Set debug_pull_request_number_str annotation"
(https://github.com/bitcoin/bitcoin/pull/33754)
(https://github.com/bitcoin/bitcoin/pull/33754)
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2486652576)
Some contention on shared resources is unavoidable. The threads need to synchronize on atomics. There are no locks in this implementation that all other threads need to wait on.
The main thread *may* have to wait here if there is a slow fetch, but it would be reading uncontested memory right up until one worker thread flips this bit. None of the other threads are trying to write to this same bit, so there is no contention between the main thread and any other worker threads.
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2486652576)
Some contention on shared resources is unavoidable. The threads need to synchronize on atomics. There are no locks in this implementation that all other threads need to wait on.
The main thread *may* have to wait here if there is a slow fetch, but it would be reading uncontested memory right up until one worker thread flips this bit. None of the other threads are trying to write to this same bit, so there is no contention between the main thread and any other worker threads.
📝 purpleKarrot opened a pull request: "prevector: simplify implementation of comparison operators and match behavior of `std::vector`"
(https://github.com/bitcoin/bitcoin/pull/33772)
The reduced amount of code reduces maintenance. Providing `prevector::operator<=>` allows classes that are implemented in terms of `prevector` to use the compiler provided `operator<=>`.
However, **there is a breaking change**!
The old implementation claims to be a drop-in replacement for `std::vector`. However, the implementation for `operator<` is different when comparing two vectors of different length, as can be presented with the following code:
```cpp
int main()
{
auto cons
...
(https://github.com/bitcoin/bitcoin/pull/33772)
The reduced amount of code reduces maintenance. Providing `prevector::operator<=>` allows classes that are implemented in terms of `prevector` to use the compiler provided `operator<=>`.
However, **there is a breaking change**!
The old implementation claims to be a drop-in replacement for `std::vector`. However, the implementation for `operator<` is different when comparing two vectors of different length, as can be presented with the following code:
```cpp
int main()
{
auto cons
...
💬 stickies-v commented on pull request "rest: Query predecessor headers using negative count param":
(https://github.com/bitcoin/bitcoin/pull/33752#issuecomment-3480837147)
The reorg potential does make using the interface more cumbersome in general, I'm not sure your approach solves that? You're still limited by what's in the `active_chain`, so you still need logic to deal with what if the index you're requesting a negative count from isn't in the active chain anymore?
(https://github.com/bitcoin/bitcoin/pull/33752#issuecomment-3480837147)
The reorg potential does make using the interface more cumbersome in general, I'm not sure your approach solves that? You're still limited by what's in the `active_chain`, so you still need logic to deal with what if the index you're requesting a negative count from isn't in the active chain anymore?
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2486669187)
I don't see why aborting early would prevent us from using less precision. It's doubtful there would be a collision, and if there were that block will just be a little slower to connect.
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2486669187)
I don't see why aborting early would prevent us from using less precision. It's doubtful there would be a collision, and if there were that block will just be a little slower to connect.
👍 ryanofsky approved a pull request: "node: add `BlockTemplateCache`"
(https://github.com/bitcoin/bitcoin/pull/33421#pullrequestreview-3411154210)
Code review ACK 084bfbc1ec7f8f64f54d231bb641285622311b59 reviewing first commit in detail, but reviewed fuzz test in second commit pretty lightly. Changes here all seem very straightforward and implmenetation seems simpler than it was initially.
I do still suspect specifying template age in seconds instead of using a more granular measure may be a mistake if this caching is supposed to be useful for mining, but others would know more about this than me. It would seem straightfoward to add anoth
...
(https://github.com/bitcoin/bitcoin/pull/33421#pullrequestreview-3411154210)
Code review ACK 084bfbc1ec7f8f64f54d231bb641285622311b59 reviewing first commit in detail, but reviewed fuzz test in second commit pretty lightly. Changes here all seem very straightforward and implmenetation seems simpler than it was initially.
I do still suspect specifying template age in seconds instead of using a more granular measure may be a mistake if this caching is supposed to be useful for mining, but others would know more about this than me. It would seem straightfoward to add anoth
...
💬 ryanofsky commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2486593971)
In commit "node: add block template cache to miner" (cd2e843907a6810c8f1acf2b7d80bbf8273a5a00)
I know I suggested `operator==` previously but since this is not checking direct equality maybe it is better to call this method something like `CanReuse` or `ReusableTemplate`. Just an idea. Keeping operator== or using any method name seems ok.
Would also suggest moving detailed comments about specific fields inisde the method implementation. Could just have a general comment like "intentionally
...
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2486593971)
In commit "node: add block template cache to miner" (cd2e843907a6810c8f1acf2b7d80bbf8273a5a00)
I know I suggested `operator==` previously but since this is not checking direct equality maybe it is better to call this method something like `CanReuse` or `ReusableTemplate`. Just an idea. Keeping operator== or using any method name seems ok.
Would also suggest moving detailed comments about specific fields inisde the method implementation. Could just have a general comment like "intentionally
...
💬 ryanofsky commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2486621134)
In commit "node: add block template cache to miner" (cd2e843907a6810c8f1acf2b7d80bbf8273a5a00)
Probably should use `uint32_t` instead of `const uint32_t&`. Const reference parameter are usually only preferred when parameter types are much bigger.
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2486621134)
In commit "node: add block template cache to miner" (cd2e843907a6810c8f1acf2b7d80bbf8273a5a00)
Probably should use `uint32_t` instead of `const uint32_t&`. Const reference parameter are usually only preferred when parameter types are much bigger.