🤔 sipa reviewed a pull request: "refactor: remove dead branches in `SingletonClusterImpl`"
(https://github.com/bitcoin/bitcoin/pull/33768#pullrequestreview-3410906177)
ACK 2d23820ee11678d567c75f94c40011ed9f0e274f
(https://github.com/bitcoin/bitcoin/pull/33768#pullrequestreview-3410906177)
ACK 2d23820ee11678d567c75f94c40011ed9f0e274f
💬 stickies-v commented on issue "log: "Replaying blocks" / "Rolling forward" logging is rate-limited":
(https://github.com/bitcoin/bitcoin/issues/33769#issuecomment-3480392336)
Is there a good reason to unconditionally log this statement? It seems to me like a single unconditional "Replaying blocks from hash (height)" before starting the loop, and a conditional (debug) per-iteration statement would be sensible?
(https://github.com/bitcoin/bitcoin/issues/33769#issuecomment-3480392336)
Is there a good reason to unconditionally log this statement? It seems to me like a single unconditional "Replaying blocks from hash (height)" before starting the loop, and a conditional (debug) per-iteration statement would be sensible?
💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486416019)
Indeed, the input 2 also gives 0 as result. The intent may have been that got indices 0 and 1 it makes no sense to jump, for the rest follow the computation (but 2 accidentally also gives 0).
In any case, I don't want to change product behavior to the slightest in this PR.
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486416019)
Indeed, the input 2 also gives 0 as result. The intent may have been that got indices 0 and 1 it makes no sense to jump, for the rest follow the computation (but 2 accidentally also gives 0).
In any case, I don't want to change product behavior to the slightest in this PR.
💬 murchandamus commented on pull request "mini miner: enable `Linearize` return package feerates":
(https://github.com/bitcoin/bitcoin/pull/33216#issuecomment-3480414914)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/33216#issuecomment-3480414914)
Concept ACK
💬 murchandamus commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-3480418753)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-3480418753)
Concept ACK
💬 murchandamus commented on pull request "rpc: print P2WSH and P2SH redem Script in getrawtransaction":
(https://github.com/bitcoin/bitcoin/pull/31252#issuecomment-3480436431)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31252#issuecomment-3480436431)
Concept ACK
💬 A-Manning commented on pull request "rest: Query predecessor headers using negative count param":
(https://github.com/bitcoin/bitcoin/pull/33752#issuecomment-3480452826)
>Request the starting hash with GET /rest/blockhashbyheight/<HEIGHT>.<bin|hex|json> and then request headers upward with /rest/headers/?
We currently use this approach at L2L. This strategy is far from ideal - any of the upward requests may return headers from a different active chain, in the event of a re-org. It is necessary to check that the final block hash is the same as the tip hash we want to sync to, and if it is not, then the only way to retrieve the headers would be requesting heade
...
(https://github.com/bitcoin/bitcoin/pull/33752#issuecomment-3480452826)
>Request the starting hash with GET /rest/blockhashbyheight/<HEIGHT>.<bin|hex|json> and then request headers upward with /rest/headers/?
We currently use this approach at L2L. This strategy is far from ideal - any of the upward requests may return headers from a different active chain, in the event of a re-org. It is necessary to check that the final block hash is the same as the tip hash we want to sync to, and if it is not, then the only way to retrieve the headers would be requesting heade
...
📝 ryanofsky opened a pull request: "init: Require explicit -asmap filename"
(https://github.com/bitcoin/bitcoin/pull/33770)
Currently, if `-asmap` is specified without a filename bitcoind tries to load `ip_asn.map` data file.
This change now requires `-asmap=ip_asn.map` or another filename to be specified explicitly.
The change is intended to make behavior of the option explicit avoid confusion reported https://github.com/bitcoin/bitcoin/issues/33386 where documentation specifies a default file which is not actually loaded by default. It was originally implemented in
https://github.com/bitcoin/bitcoin/pull/336
...
(https://github.com/bitcoin/bitcoin/pull/33770)
Currently, if `-asmap` is specified without a filename bitcoind tries to load `ip_asn.map` data file.
This change now requires `-asmap=ip_asn.map` or another filename to be specified explicitly.
The change is intended to make behavior of the option explicit avoid confusion reported https://github.com/bitcoin/bitcoin/issues/33386 where documentation specifies a default file which is not actually loaded by default. It was originally implemented in
https://github.com/bitcoin/bitcoin/pull/336
...
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2486501704)
Hmm right I was not clearing the containers, so the sorting was dominating. I retried with clearing and the unordered_map is still slightly faster. In your benchmark you are creating and reserving the map inside the benchmark instead of before. In this implementation the reserved memory is kept in between blocks, so reserving and creating outside makes more sense.
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2486501704)
Hmm right I was not clearing the containers, so the sorting was dominating. I retried with clearing and the unordered_map is still slightly faster. In your benchmark you are creating and reserving the map inside the benchmark instead of before. In this implementation the reserved memory is kept in between blocks, so reserving and creating outside makes more sense.
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2486506578)
Why would short ids matter here though? Isn't that for keeping the bandwidth smaller for compact blocks?
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2486506578)
Why would short ids matter here though? Isn't that for keeping the bandwidth smaller for compact blocks?
💬 ryanofsky commented on pull request "init: Split file path handling out of -asmap option":
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3480556829)
https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3475259644
> The long term plan is to have the embedded data used by default, however first the idea was to have a release or two with the data embedded but the usage still off by default.
This makes sense, but I think if it is the plan, it should rule out option "1. Use the default file when it's present" so `bitcoind` will consistently just not load asmap data, instead of loading a file by default but not loading embedded data by
...
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3480556829)
https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3475259644
> The long term plan is to have the embedded data used by default, however first the idea was to have a release or two with the data embedded but the usage still off by default.
This makes sense, but I think if it is the plan, it should rule out option "1. Use the default file when it's present" so `bitcoind` will consistently just not load asmap data, instead of loading a file by default but not loading embedded data by
...
💬 l0rinc commented on issue "log: "Replaying blocks" / "Rolling forward" logging is rate-limited":
(https://github.com/bitcoin/bitcoin/issues/33769#issuecomment-3480558074)
I have pushed something similar in https://github.com/bitcoin/bitcoin/pull/33443
(https://github.com/bitcoin/bitcoin/issues/33769#issuecomment-3480558074)
I have pushed something similar in https://github.com/bitcoin/bitcoin/pull/33443
💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2486519559)
no, I mean, we don't actually need 256 bits of precision here, just a probabilitistic check, so taking the first 32/64 bits of the hash should suffice, since the worst case is just going to disk, so it's not a tragedy if there are false positives, as long as in the average case checks are lot faster
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2486519559)
no, I mean, we don't actually need 256 bits of precision here, just a probabilitistic check, so taking the first 32/64 bits of the hash should suffice, since the worst case is just going to disk, so it's not a tragedy if there are false positives, as long as in the average case checks are lot faster
🤔 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])
```