👍 instagibbs approved a pull request: "cluster mempool: optimized candidate search"
(https://github.com/bitcoin/bitcoin/pull/30286#pullrequestreview-2251100080)
code review ACK 1b8f143fb791fe5ba40ce6610cd4e8e04b951495
Thanks for dropping those couple commits.
> I think this will need analysis with real-world clusters to see actual impact, which may be easier once the cluster mempool project is further along. I've dropped it for this reason.
On that note I think we'll need some diagram-quality style benchmarks to make informed decisions in the future, otherwise it makes ongoing changes difficult to justify.
(https://github.com/bitcoin/bitcoin/pull/30286#pullrequestreview-2251100080)
code review ACK 1b8f143fb791fe5ba40ce6610cd4e8e04b951495
Thanks for dropping those couple commits.
> I think this will need analysis with real-world clusters to see actual impact, which may be easier once the cluster mempool project is further along. I've dropped it for this reason.
On that note I think we'll need some diagram-quality style benchmarks to make informed decisions in the future, otherwise it makes ongoing changes difficult to justify.
💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1725236618)
a12ca7b8d99d868ff49e90d33b5e90c169849ccc
may want to consider something like `m_sorted_depgraph`
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1725236618)
a12ca7b8d99d868ff49e90d33b5e90c169849ccc
may want to consider something like `m_sorted_depgraph`
💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1725257738)
same check already being done in split_fn, but I also like it here on creation
```Suggestion
inc(std::move(i)), und(std::move(u)), pot_feerate(std::move(p_f)) {
Assume(pot_feerate.IsEmpty() == inc.feerate.IsEmpty());
}
```
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1725257738)
same check already being done in split_fn, but I also like it here on creation
```Suggestion
inc(std::move(i)), und(std::move(u)), pot_feerate(std::move(p_f)) {
Assume(pot_feerate.IsEmpty() == inc.feerate.IsEmpty());
}
```
💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1725534553)
new name is better and makes flow easier to understand,thanks
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1725534553)
new name is better and makes flow easier to understand,thanks
💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1725548581)
:facepalm: I was missing the `static` in front of `prevout_hash`. This is making a lot more sense now.
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1725548581)
:facepalm: I was missing the `static` in front of `prevout_hash`. This is making a lot more sense now.
💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1725548968)
might be worth a comment!
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1725548968)
might be worth a comment!
💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1725555941)
Done.
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1725555941)
Done.
💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1725556045)
Done.
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1725556045)
Done.
💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2302743849)
> On that note I think we'll need some diagram-quality style benchmarks to make informed decisions in the future, otherwise it makes ongoing changes difficult to justify.
FWIW, the two dropped commits are pure optimizations (they should have zero effect on the outcome, or the number of iterations performed; just on the amount of time spent per iteration).
I'll add an overview of the expected impact of each commit in this PR description.
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2302743849)
> On that note I think we'll need some diagram-quality style benchmarks to make informed decisions in the future, otherwise it makes ongoing changes difficult to justify.
FWIW, the two dropped commits are pure optimizations (they should have zero effect on the outcome, or the number of iterations performed; just on the amount of time spent per iteration).
I'll add an overview of the expected impact of each commit in this PR description.
💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2302746644)
@sipa sure, but if we're weighing a change to this, it still matters as it may modify how many iterations we're willing to do
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2302746644)
@sipa sure, but if we're weighing a change to this, it still matters as it may modify how many iterations we're willing to do
🤔 instagibbs reviewed a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2251681805)
reviewed through bbec8e261467637ce26ecf551b528d9061cf4ffe
via `git range-diff master 388c2f5d7ee5f38cbefaff0c85cf298083127299 bbec8e261467637ce26ecf551b528d9061cf4ffe`
LGTM aside from waiting on fuzzer coverage results + fixes
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2251681805)
reviewed through bbec8e261467637ce26ecf551b528d9061cf4ffe
via `git range-diff master 388c2f5d7ee5f38cbefaff0c85cf298083127299 bbec8e261467637ce26ecf551b528d9061cf4ffe`
LGTM aside from waiting on fuzzer coverage results + fixes
💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1725592055)
nit: just assert it's in orphanage instead
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1725592055)
nit: just assert it's in orphanage instead
💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2302806417)
reACK 12760a57b3a6cd1aeb3b7532311f648de2b45aa4
via `git range-diff master 1b8f143fb791fe5ba40ce6610cd4e8e04b951495 12760a57b3a6cd1aeb3b7532311f648de2b45aa4`
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2302806417)
reACK 12760a57b3a6cd1aeb3b7532311f648de2b45aa4
via `git range-diff master 1b8f143fb791fe5ba40ce6610cd4e8e04b951495 12760a57b3a6cd1aeb3b7532311f648de2b45aa4`
💬 fjahr commented on pull request "Have miner account for timewarp mitigation, activate on regtest, lower nPowTargetTimespan to 144 and add test":
(https://github.com/bitcoin/bitcoin/pull/30681#issuecomment-2302873162)
ACK 59ff17e5af4e382cbe16f183767beef1bdcd9131
Reviewed the code and verified that the newly added tests do actually test the change in behavior.
(https://github.com/bitcoin/bitcoin/pull/30681#issuecomment-2302873162)
ACK 59ff17e5af4e382cbe16f183767beef1bdcd9131
Reviewed the code and verified that the newly added tests do actually test the change in behavior.
💬 maflcko commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1725663019)
Not sure about this. To catch the seed, logging should be enabled (and it is enabled in all CI tasks). (This isn't changed in this pull request, so the comment seems tangential)
If the burden is put on each test that uses any kind of randomness to capture and memorize the seed, and print it on failure, it will be spotty at best and confusing, verbose and inconsistent in general.
If you are worried that the seed could be missed, I share your concern, but a better or more general fix would b
...
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1725663019)
Not sure about this. To catch the seed, logging should be enabled (and it is enabled in all CI tasks). (This isn't changed in this pull request, so the comment seems tangential)
If the burden is put on each test that uses any kind of randomness to capture and memorize the seed, and print it on failure, it will be spotty at best and confusing, verbose and inconsistent in general.
If you are worried that the seed could be missed, I share your concern, but a better or more general fix would b
...
💬 hebasto commented on pull request "build: Mark `x86_64-linux-gnu` release binaries as CET-enabled":
(https://github.com/bitcoin/bitcoin/pull/30685#issuecomment-2302894655)
> Concept NACK on the approach here (CI is showing why it wont work with depends, I assume it will also break other builds (where we haven't built the libs)). I'd rather build up from from something like #30438 (and have a branch with similar changes).
1. CI is green.
2. This approach works and fixes [#30677](https://github.com/bitcoin/bitcoin/issues/30677).
3. [#30438](https://github.com/bitcoin/bitcoin/issues/30438) does not indicate that it is capable of resolving [#30677](https://github
...
(https://github.com/bitcoin/bitcoin/pull/30685#issuecomment-2302894655)
> Concept NACK on the approach here (CI is showing why it wont work with depends, I assume it will also break other builds (where we haven't built the libs)). I'd rather build up from from something like #30438 (and have a branch with similar changes).
1. CI is green.
2. This approach works and fixes [#30677](https://github.com/bitcoin/bitcoin/issues/30677).
3. [#30438](https://github.com/bitcoin/bitcoin/issues/30438) does not indicate that it is capable of resolving [#30677](https://github
...
💬 maflcko commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1725675521)
Personally, I like how it is done in the python functional tests:
* Allow to log it to the terminal (This can be used by developers)
* Always record the seed in the log in the test temp dir, which is preserved on failure, so that it can be looked up afterwards. (This is used by CI and can be used by developers as well)
Doing the same in the unit tests would be nice, but again, this seems best to be tracked, discussed and fixed in a separate issue or pull request.
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1725675521)
Personally, I like how it is done in the python functional tests:
* Allow to log it to the terminal (This can be used by developers)
* Always record the seed in the log in the test temp dir, which is preserved on failure, so that it can be looked up afterwards. (This is used by CI and can be used by developers as well)
Doing the same in the unit tests would be nice, but again, this seems best to be tracked, discussed and fixed in a separate issue or pull request.
💬 sipa commented on issue "Control-flow application capabilities for `x86_64-linux-gnu` release binaries":
(https://github.com/bitcoin/bitcoin/issues/30677#issuecomment-2302905383)
Does the ELF flag control whether the feature will be enabled at runtime?
(https://github.com/bitcoin/bitcoin/issues/30677#issuecomment-2302905383)
Does the ELF flag control whether the feature will be enabled at runtime?
💬 fanquake commented on pull request "build: Mark `x86_64-linux-gnu` release binaries as CET-enabled":
(https://github.com/bitcoin/bitcoin/pull/30685#discussion_r1725686630)
I don't think this is the right place for these flags. If we want them in the Guix build, then we should add them to the Guix build, not hardcode them into depends.
(https://github.com/bitcoin/bitcoin/pull/30685#discussion_r1725686630)
I don't think this is the right place for these flags. If we want them in the Guix build, then we should add them to the Guix build, not hardcode them into depends.
👋 fanquake's pull request is ready for review: "[27.x] Even more backports"
(https://github.com/bitcoin/bitcoin/pull/30558)
(https://github.com/bitcoin/bitcoin/pull/30558)