Bitcoin Core Github
43 subscribers
123K links
Download Telegram
💬 maflcko commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1763536070)
Ok, done
💬 andrewtoth commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1763544909)
I still think this should be an enum named `EntryState` or something, with all combinations of flags like https://github.com/bitcoin/bitcoin/pull/30906/commits/ba52ebd41673abf63d4495215c84be7244469d30#r1760349038. That's the proper way to do this instead of defining static vars for each value of a char we care about.
💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1763548746)
Absolutely, that's ny plan for tomorrow - but needed to clean up the test before doing that
💬 maflcko commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#issuecomment-2356432684)
> Agree with ryanofsky that it would be good to document rough compile time impact if any.

Sure added a note that it is less than 1% of both memory and time. The time should be offset by dropping the corresponding stuff from the Python linter, which is slower than a compiler at iterating over strings.
💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1763454509)
Thanks! Taken.
💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1763448694)
Dropped this test for now, and left a todo to add more tests to exercise different methods of mempool submission and ensure we never violate the cluster limits.
💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1763330087)
Removed (it was needed in an intermediate commit).
💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1761997599)
Done.
💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1763536059)
Taken.
💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1761997443)
good point. i wonder if we should just drop the feerate diagram language altogether and leave it at "insufficient fee"?
💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1763532380)
Thanks! Taken.
💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1763554660)
Thanks, removed.
💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1763546504)
Ah, right. Given that this falls back to the vsize limit being exercised, I think this isn't really worth including? Will leave this for now.
👋 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?