👍 tdb3 approved a pull request: "refactor: add coinbase constraints to BlockAssembler::Options"
(https://github.com/bitcoin/bitcoin/pull/30356#pullrequestreview-2181599489)
Code Review ACK 95c2edefe383c2b8a1735c26d8442fd36fa5e339
Good addition and the updates tidy things up nicely.
I would also support a bitcoind config parameter for this.
(https://github.com/bitcoin/bitcoin/pull/30356#pullrequestreview-2181599489)
Code Review ACK 95c2edefe383c2b8a1735c26d8442fd36fa5e339
Good addition and the updates tidy things up nicely.
I would also support a bitcoind config parameter for this.
🤔 kevkevinpal reviewed a pull request: "fuzz: add target for `CoinsResult`"
(https://github.com/bitcoin/bitcoin/pull/30461#pullrequestreview-2181679638)
Concept ACK [385ec3f](https://github.com/bitcoin/bitcoin/pull/30461/commits/385ec3f12b6cacfa92aa2bc9ffb0177f3120adca)
I see value in adding fuzz coverage to `CoinsResult` `All`, `Erase`, and `Clear`
(https://github.com/bitcoin/bitcoin/pull/30461#pullrequestreview-2181679638)
Concept ACK [385ec3f](https://github.com/bitcoin/bitcoin/pull/30461/commits/385ec3f12b6cacfa92aa2bc9ffb0177f3120adca)
I see value in adding fuzz coverage to `CoinsResult` `All`, `Erase`, and `Clear`
💬 kevkevinpal commented on pull request "fuzz: add target for `CoinsResult`":
(https://github.com/bitcoin/bitcoin/pull/30461#discussion_r1680278518)
does it make sense to add another `CallOneOf` with just `coin_result.Clear()` because aren't we forcing the pattern of `Clear()` -> `All()` -> `Size()` and never `Clear()` -> `Size()`
not sure if that is needed, just a thought
(https://github.com/bitcoin/bitcoin/pull/30461#discussion_r1680278518)
does it make sense to add another `CallOneOf` with just `coin_result.Clear()` because aren't we forcing the pattern of `Clear()` -> `All()` -> `Size()` and never `Clear()` -> `Size()`
not sure if that is needed, just a thought
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680326042)
If `accumulator` is equal in feerate to `subset`, but smaller in size, it would be correct to either return `accumulator` (as done in the current code) or to continue the loop. Returning `best` in this case would not be correct, as it's possible that a bigger intersection still has a higher feerate.
In general, I prefer returning the first (smallest) acceptable intersection because that is the least amount of iteration work, and it does not hurt quality (if a bigger intersection of higher fee
...
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680326042)
If `accumulator` is equal in feerate to `subset`, but smaller in size, it would be correct to either return `accumulator` (as done in the current code) or to continue the loop. Returning `best` in this case would not be correct, as it's possible that a bigger intersection still has a higher feerate.
In general, I prefer returning the first (smallest) acceptable intersection because that is the least amount of iteration work, and it does not hurt quality (if a bigger intersection of higher fee
...
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680336040)
No, we need to invoke `MarkDone` even when the `iterations_done_now == max_iterations_now` condition is not satisfied.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680336040)
No, we need to invoke `MarkDone` even when the `iterations_done_now == max_iterations_now` condition is not satisfied.
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680345658)
Does looking at the documented serialization in the unit tests help? If not, I will add a worked-out example in more detail to the `DepGraphFormatter` documentation.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680345658)
Does looking at the documented serialization in the unit tests help? If not, I will add a worked-out example in more detail to the `DepGraphFormatter` documentation.
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680345744)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680345744)
Fixed.
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680345825)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680345825)
Fixed.
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680345874)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680345874)
Fixed.
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680345931)
Done.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680345931)
Done.
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680345987)
Done.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680345987)
Done.
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680346568)
Done, and added more consistency checks on top (chunks must be non-empty, match the highest-feerate remaining prefix, and be topological).
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680346568)
Done, and added more consistency checks on top (chunks must be non-empty, match the highest-feerate remaining prefix, and be topological).
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680347774)
Good question.
Clusters, as intended to be used by the mempool/txgraph code, are always connected (if not, they'd be separate clusters), but nothing actually relies on this property, so all cluster_linearization tests should pass even with non-connecting clusters. I believe that earlier it was helpful to forcefully make all clusters for most fuzz tests connected, as this helped the fuzzer find extreme cases, though I have not tested this recently.
This is also the reason why this commit is
...
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1680347774)
Good question.
Clusters, as intended to be used by the mempool/txgraph code, are always connected (if not, they'd be separate clusters), but nothing actually relies on this property, so all cluster_linearization tests should pass even with non-connecting clusters. I believe that earlier it was helpful to forcefully make all clusters for most fuzz tests connected, as this helped the fuzzer find extreme cases, though I have not tested this recently.
This is also the reason why this commit is
...
💬 maflcko commented on pull request "doc: getaddressinfo[isscript] is optional":
(https://github.com/bitcoin/bitcoin/pull/30457#discussion_r1680403465)
thanks, fixed
(https://github.com/bitcoin/bitcoin/pull/30457#discussion_r1680403465)
thanks, fixed
💬 maflcko commented on pull request "doc: getaddressinfo[isscript] is optional":
(https://github.com/bitcoin/bitcoin/pull/30457#discussion_r1680403780)
Left the nit for now.
(https://github.com/bitcoin/bitcoin/pull/30457#discussion_r1680403780)
Left the nit for now.
💬 maflcko commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680414541)
not sure about overloading this. Otherwise, one may get a runtime evaluation when switching from a raw string literal pointer to a consteval string_view, no?
Seems clearer to make it a constructor taking a string_view? I guess the only confusion could be that the `Span<const unsigned char>` and string_view (aka `Span<const char>`) do different things (one is hex decoding and the other is not), but that should be fine, because both are distinct types.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680414541)
not sure about overloading this. Otherwise, one may get a runtime evaluation when switching from a raw string literal pointer to a consteval string_view, no?
Seems clearer to make it a constructor taking a string_view? I guess the only confusion could be that the `Span<const unsigned char>` and string_view (aka `Span<const char>`) do different things (one is hex decoding and the other is not), but that should be fine, because both are distinct types.
💬 maflcko commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680415419)
Same
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680415419)
Same
💬 maflcko commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680415361)
What is the point of this? Seems odd to add space before compilation and then remove it during compilation.
Seems easier to not add the space in the first place and fail compilation when there is one?
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680415361)
What is the point of this? Seems odd to add space before compilation and then remove it during compilation.
Seems easier to not add the space in the first place and fail compilation when there is one?
💬 maflcko commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680419847)
Not sure about hardcoding this for every base_blob.
Seems easier to just implement it once for `base_blob` and then have it available for all?
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680419847)
Not sure about hardcoding this for every base_blob.
Seems easier to just implement it once for `base_blob` and then have it available for all?
💬 maflcko commented on pull request "refactor: Make uint256S(const char*) and ParseHex(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680422462)
Not sure about fuzzy decoding here. Accidentally truncating an `uint256` and thus parsing it as something else seems dangerous.
The convenience of being able to truncate leading zeros of a hex-encoded base_blob are never used, are they? The two constants ZERO and ONE are constructed without hex-decoding, so this isn't needed there, and I fail to see another use case right now.
It seems easier to just assert WIDTH.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1680422462)
Not sure about fuzzy decoding here. Accidentally truncating an `uint256` and thus parsing it as something else seems dangerous.
The convenience of being able to truncate leading zeros of a hex-encoded base_blob are never used, are they? The two constants ZERO and ONE are constructed without hex-decoding, so this isn't needed there, and I fail to see another use case right now.
It seems easier to just assert WIDTH.