Bitcoin Core Github
43 subscribers
123K links
Download Telegram
👋 maflcko's pull request is ready for review: "ci: Use clang-19 in msan tasks"
(https://github.com/bitcoin/bitcoin/pull/30639)
🤔 jonatack reviewed a pull request: "Drop -dbcache limit"
(https://github.com/bitcoin/bitcoin/pull/28358#pullrequestreview-2310356555)
ACK bb3b980dfd96d9a89e6b04aef2859ce0555c6807

Agee with more freedom/responsibility for the user and the ability to reduce flushing to disk for better IBD performance.

It's probably a good idea to avoid using strike-through tags in PR descriptions, as they aren't rendered well by the merge script and look like emphasis instead.

Due to recent UTXO set growth, the current maximum value for `-dbcache` of 16GB is ~just months away from being~ insufficient (for those who wish to complete
...
🤔 instagibbs reviewed a pull request: "cluster mempool: extend DepGraph functionality"
(https://github.com/bitcoin/bitcoin/pull/30857#pullrequestreview-2306885844)
First run through.

suggestion for clusterlin_add_dependencies harness: https://github.com/instagibbs/bitcoin/commit/c5c44da5a0a630a6725ca85e90e4c67b2eb1f8bc

I think this gets similar or better coverage and is easier for me to understand?

Tried augmenting `RemoveTransactions` coverage with adding things back in post deletion, and it's tripping up SanityCheck. Take a look?

```
diff --git a/src/test/fuzz/cluster_linearize.cpp b/src/test/fuzz/cluster_linearize.cpp
index be06898a6e..05d
...
💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1761348362)
?
```Suggestion
assert((depgraph.Descendants(child) & children) == SetType::Singleton(child));
```
💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1761406415)
nit
```Suggestion
// Compute the ancestors of parents that are not already ancestors of child.
```
💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1761301820)
commit 77e07eba11391868f83db22bf2280e02b5a8cd78

add a motivation for these additions in commit message?
💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1761676550)
I don't see where these are being deleted? The only removal below is removing things that aren't in `Positions()`, which the newly added transactions should be? Likely misreading this.
💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1761630898)
in commit message for 7510a65127db610df4be9cdd95eaef0c23b3e2ff

"This commits starts introducing support" are we expecting more support later?
💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1761348880)
?
```Suggestion
assert((depgraph.Ancestors(parent) & parents) == SetType::Singleton(parent));
```
💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1763548308)
Ergonomically speaking, I expect this to be used to remove a set of "direct conflicts" and all descendants of that set, so this wouldn't cause invalid views of the underlying cluster's graph relationships.

Is this correct?
💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1763550167)
do we want to leave a note that now-unused entries not at the end can be non-empty/dirty? If that's not possible, a comment explaining that would be helpful too.
💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1761452737)
could use this in AddDependency as a one-liner?
```
AddDependencies(SetType::Singleton(parent), child);
```
📝 marcofleon opened a pull request: "fuzz: Add check in `p2p_headers_presync` that chain work never exceeds minimum work"
(https://github.com/bitcoin/bitcoin/pull/30918)
A followup to https://github.com/bitcoin/bitcoin/pull/30661

The added assertion just makes sure that the fuzz test is working as intended. If we're sure that the total work of the test chain is never more than minimum chain work, then we can be sure that the later assertion failure would actually mean that a bug in the headers presync logic was found.
💬 marcofleon commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2356502855)
> Another follow up improvement I'd like is direct assertion that the "chainwork" of the header chain never exceed minimum chainwork, or if that is exceeded, we can drop the assertion that the chain never extends past genesis block.

Followup here: https://github.com/bitcoin/bitcoin/pull/30918

Also, I was mistaken in this [comment](https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1750661750). The test as is already does build a chain that is more than 16 headers long. So, there was
...
💬 jonatack commented on pull request "doc: remove Eclipser fuzzing documentation":
(https://github.com/bitcoin/bitcoin/pull/30908#issuecomment-2356532534)
> I think we could even remove eclipser from the docs entirely. As you noted as well, it looks unmaintained and I don't think any of us are actively using it.

Done, thanks everyone for the feedback. Leaving for a follow-up the addition of a concolic testing tool example with bug.
💬 maflcko commented on pull request "doc: remove Eclipser fuzzing documentation":
(https://github.com/bitcoin/bitcoin/pull/30908#issuecomment-2356545824)
review ACK 735436df8cebf28d4bcf0ad7e8f1fd7f19191001
👍 brunoerg approved a pull request: "doc: remove Eclipser fuzzing documentation"
(https://github.com/bitcoin/bitcoin/pull/30908#pullrequestreview-2310438395)
ACK 735436df8cebf28d4bcf0ad7e8f1fd7f19191001
💬 marcofleon commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1763652769)
Why is it that we don't initialize these with the`Txid` and `Wtxid` types? Have been thinking about making `RelayTransaction` type safe, so just wondering.
👍 theuni approved a pull request: "ci: Use `ninja` to build in macOS native CI job"
(https://github.com/bitcoin/bitcoin/pull/30915#pullrequestreview-2310455658)
ACK d01b85bfecbcf16ea16f90e2ade7537bf582269f.

I was going to suggest this as well :)
💬 jonatack commented on pull request "ci: Use `ninja` to build in macOS native CI job":
(https://github.com/bitcoin/bitcoin/pull/30915#issuecomment-2356604457)
Concept ACK in using https://ninja-build.org / https://github.com/ninja-build/ninja for a CI task as a point of comparison.