💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2534735892)
In commit "Add transactions to txgraph, but without cluster dependencies"
Nit: add an `AssertLockHeld(m_pool.cs);` here too?
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2534735892)
In commit "Add transactions to txgraph, but without cluster dependencies"
Nit: add an `AssertLockHeld(m_pool.cs);` here too?
💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2535009153)
In commit "Select transactions for blocks based on chunk feerate"
Nit: now that the chunk finding logic is inside txgraph, there is no actual heap anymore (it internally just iterates the feerate-sorted chunk index, but that's an implementation detail).
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2535009153)
In commit "Select transactions for blocks based on chunk feerate"
Nit: now that the chunk finding logic is inside txgraph, there is no actual heap anymore (it internally just iterates the feerate-sorted chunk index, but that's an implementation detail).
💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2535138491)
In commit " [squashme] Use outputs and structured bindings for CalculateAncestorD…"
Squash the squashme?
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2535138491)
In commit " [squashme] Use outputs and structured bindings for CalculateAncestorD…"
Squash the squashme?
💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2535130273)
In commit "Use cluster linearization for transaction relay sort order"
Nit: if "mining score" means "chunk feerate", then this description is not complete. There is also the possibility of a and b having the same chunk feerate, being in distinct clusters, and a's cluster being given priority.
Though, I think all of this can be treated as a TxGraph implementation detail now. What about:
```
/* Return `true` if hasha should be considered sooner than hashb, namely when:
* a is not i
...
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2535130273)
In commit "Use cluster linearization for transaction relay sort order"
Nit: if "mining score" means "chunk feerate", then this description is not complete. There is also the possibility of a and b having the same chunk feerate, being in distinct clusters, and a's cluster being given priority.
Though, I think all of this can be treated as a TxGraph implementation detail now. What about:
```
/* Return `true` if hasha should be considered sooner than hashb, namely when:
* a is not i
...
💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2535342513)
In commit "fuzz: remove comparison between mini_miner block construction and miner"
Ok. This commit belongs much earlier in the PR, though (just before "Select transactions for blocks based on chunk feerate"). Otherwise, the range of commits in between will fail fuzz tests if run long enough.
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2535342513)
In commit "fuzz: remove comparison between mini_miner block construction and miner"
Ok. This commit belongs much earlier in the PR, though (just before "Select transactions for blocks based on chunk feerate"). Otherwise, the range of commits in between will fail fuzz tests if run long enough.
💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2535291747)
In commit "squashme: only output weight, not vsize, for cluster/chunk information"
Squash the squashme?
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2535291747)
In commit "squashme: only output weight, not vsize, for cluster/chunk information"
Squash the squashme?
💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2535278410)
In commit "Rework RBF and TRUC validation"
The `GetParents` function is only introduced in the next commit.
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2535278410)
In commit "Rework RBF and TRUC validation"
The `GetParents` function is only introduced in the next commit.
📝 joshdoman opened a pull request: "[kernel] Expose reusable `PrecomputedTransactionData` in script validation"
(https://github.com/bitcoin/bitcoin/pull/33891)
This PR exposes a reusable `PrecomputedTransactionData` object in script validation using libkernel.
Currently, libkernel computes `PrecomputedTransactionData` each time `btck_script_pubkey_verify` is called, exposing clients to quadratic hashing when validating a transaction with multiple inputs. By externalizing `PrecomputedTransactionData` and making it reusable, libkernel can eliminate this attack vector.
I discussed this problem in [this issue](https://github.com/TheCharlatan/rust-bit
...
(https://github.com/bitcoin/bitcoin/pull/33891)
This PR exposes a reusable `PrecomputedTransactionData` object in script validation using libkernel.
Currently, libkernel computes `PrecomputedTransactionData` each time `btck_script_pubkey_verify` is called, exposing clients to quadratic hashing when validating a transaction with multiple inputs. By externalizing `PrecomputedTransactionData` and making it reusable, libkernel can eliminate this attack vector.
I discussed this problem in [this issue](https://github.com/TheCharlatan/rust-bit
...
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2535364299)
> Not sure why we'd want to reduce dependencies here, what's the advantage of that?
I wrote it above; the idea is to be able to use this low-level class and tests elsewhere, outside the project, just by pulling a few files without dragging in all our unit test framework machinery, which has tons other dependencies.
If we were talking about a substantial improvement, I’d agree with you, but here it’s just a 2-line diff with no behavior change. And for me, that makes the rationale for includin
...
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2535364299)
> Not sure why we'd want to reduce dependencies here, what's the advantage of that?
I wrote it above; the idea is to be able to use this low-level class and tests elsewhere, outside the project, just by pulling a few files without dragging in all our unit test framework machinery, which has tons other dependencies.
If we were talking about a substantial improvement, I’d agree with you, but here it’s just a 2-line diff with no behavior change. And for me, that makes the rationale for includin
...
💬 plebhash commented on issue "[`v30.0rc3`]`bitcoin-node` aborts with mining IPC interface usage":
(https://github.com/bitcoin/bitcoin/issues/33554#issuecomment-3543642952)
@ryanofsky should we close this?
(https://github.com/bitcoin/bitcoin/issues/33554#issuecomment-3543642952)
@ryanofsky should we close this?
💬 achow101 commented on pull request "wallet: Replace CWalletTx::mapValue and vOrderForm with explicit class members":
(https://github.com/bitcoin/bitcoin/pull/32763#discussion_r2535396633)
Done
(https://github.com/bitcoin/bitcoin/pull/32763#discussion_r2535396633)
Done
💬 mzumsande commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2535398529)
had a closer look now, it actually is straightforward using `GetDebugMessage()`. So I added the log.
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2535398529)
had a closer look now, it actually is straightforward using `GetDebugMessage()`. So I added the log.
💬 mzumsande commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#issuecomment-3543689545)
[6d2c8ea](https://github.com/bitcoin/bitcoin/commit/6d2c8ea9dbd77c71051935b5ab59224487509559) to [6db2551](https://github.com/bitcoin/bitcoin/commit/6db2551dc27c4a9b989e8814054c93dd9d8f1b36):
Added hash of invalid block to the log.
(https://github.com/bitcoin/bitcoin/pull/33553#issuecomment-3543689545)
[6d2c8ea](https://github.com/bitcoin/bitcoin/commit/6d2c8ea9dbd77c71051935b5ab59224487509559) to [6db2551](https://github.com/bitcoin/bitcoin/commit/6db2551dc27c4a9b989e8814054c93dd9d8f1b36):
Added hash of invalid block to the log.
📝 instagibbs opened a pull request: "policy: Remove individual transaction <minrelay restriction"
(https://github.com/bitcoin/bitcoin/pull/33892)
Prior to cluster mempool, a policy was in place that
disallowed non-TRUC transactions from being
TX_RECONSIDERABLE in a package setting if it was below
minrelay. This was meant to simplify reasoning about mempool
trimming requirements with non-trivial transaction
topologies in the mempool. This is no longer a concern
post-cluster mempool, so this is relaxed.
In effect, this makes 0-value parent transactions relayable
through the network without the TRUC restrictions and
thus the a
...
(https://github.com/bitcoin/bitcoin/pull/33892)
Prior to cluster mempool, a policy was in place that
disallowed non-TRUC transactions from being
TX_RECONSIDERABLE in a package setting if it was below
minrelay. This was meant to simplify reasoning about mempool
trimming requirements with non-trivial transaction
topologies in the mempool. This is no longer a concern
post-cluster mempool, so this is relaxed.
In effect, this makes 0-value parent transactions relayable
through the network without the TRUC restrictions and
thus the a
...
💬 hodlinator commented on pull request "build: Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2535330614)
Good point regarding .clang-format, though that value from 13f36c020f0329b5e975282b45292fdf2a495e31 comes from clang-format defaults rather than a conscious decision on our part. (It will not remove newlines at EOF if they exist though, https://releases.llvm.org/16.0.0/tools/clang/docs/ClangFormatStyleOptions.html#insertnewlineateof).
GitHub displays a red warning symbol in diffs when one removes the empty newline at ends of files.
`git apply diff.patch` fails for...
```diff
diff --git a
...
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2535330614)
Good point regarding .clang-format, though that value from 13f36c020f0329b5e975282b45292fdf2a495e31 comes from clang-format defaults rather than a conscious decision on our part. (It will not remove newlines at EOF if they exist though, https://releases.llvm.org/16.0.0/tools/clang/docs/ClangFormatStyleOptions.html#insertnewlineateof).
GitHub displays a red warning symbol in diffs when one removes the empty newline at ends of files.
`git apply diff.patch` fails for...
```diff
diff --git a
...
💬 hodlinator commented on pull request "build: Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2535372567)
Thanks! (I believe that's fixed in this PR - 093980af8e4e594716b9219931fd441266c1fcd4 "init, net: Implement usage of binary-embedded asmap data")
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2535372567)
Thanks! (I believe that's fixed in this PR - 093980af8e4e594716b9219931fd441266c1fcd4 "init, net: Implement usage of binary-embedded asmap data")
💬 hodlinator commented on pull request "build: Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2535368296)
Ah, wasn't on my radar, cheers! Does simplify things a bit.
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2535368296)
Ah, wasn't on my radar, cheers! Does simplify things a bit.
💬 instagibbs commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3543708460)
https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2432472005 still has invalidateblock in `test/functional/mempool_packages.py` ? If you don't have the time to carry the patch I can take over, just let me know.
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3543708460)
https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2432472005 still has invalidateblock in `test/functional/mempool_packages.py` ? If you don't have the time to carry the patch I can take over, just let me know.
🤔 hebasto reviewed a pull request: "refactor: Avoid -W*-whitespace in git archive"
(https://github.com/bitcoin/bitcoin/pull/33869#pullrequestreview-3474450076)
post-merge ACK.
(https://github.com/bitcoin/bitcoin/pull/33869#pullrequestreview-3474450076)
post-merge ACK.
🤔 glozow reviewed a pull request: "Cluster mempool"
(https://github.com/bitcoin/bitcoin/pull/33629#pullrequestreview-3455337470)
Code reviewed the "squashme"s (I think they should be squashed) and the end state for MemPoolAccept, miner, reorg, src/policy/* changes. I plan to put most of my comments on #33591. My suggestions here are to clean up the commits so the git history is easy to trace in the future.
(https://github.com/bitcoin/bitcoin/pull/33629#pullrequestreview-3455337470)
Code reviewed the "squashme"s (I think they should be squashed) and the end state for MemPoolAccept, miner, reorg, src/policy/* changes. I plan to put most of my comments on #33591. My suggestions here are to clean up the commits so the git history is easy to trace in the future.