🤔 glozow reviewed a pull request: "cluster mempool: cluster linearization algorithm"
(https://github.com/bitcoin/bitcoin/pull/30126#pullrequestreview-2118340933)
Looked at the first 4 commits so far, just doing code review and trying the algos out in `MiniMiner`.
(https://github.com/bitcoin/bitcoin/pull/30126#pullrequestreview-2118340933)
Looked at the first 4 commits so far, just doing code review and trying the algos out in `MiniMiner`.
💬 glozow commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1639799588)
In the docs, maybe instead of "best remaining ancestor set", say "highest feerate ancestor set" ?
Side note - I was thinking that this was a replica of the ancestor set algo in `BlockAssembler` but this isn't the minimum of individual and ancestor feerate.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1639799588)
In the docs, maybe instead of "best remaining ancestor set", say "highest feerate ancestor set" ?
Side note - I was thinking that this was a replica of the ancestor set algo in `BlockAssembler` but this isn't the minimum of individual and ancestor feerate.
💬 glozow commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1668545670)
304d3cb23ba9f084b98f9f29b47e3dfbb61ca334
may be helpful context
```suggestion
// Never MarkDone the same transaction more than once as this function
// blindly subtracts the transaction's feerate from m_ancestor_set_feerates
Assume(select.IsSubsetOf(m_todo));
```
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1668545670)
304d3cb23ba9f084b98f9f29b47e3dfbb61ca334
may be helpful context
```suggestion
// Never MarkDone the same transaction more than once as this function
// blindly subtracts the transaction's feerate from m_ancestor_set_feerates
Assume(select.IsSubsetOf(m_todo));
```
💬 glozow commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1668665030)
For each of the `CandidateFinder`s, would it be helpful to expose a `AllDone()` function? Seems slightly inconvenient for the caller to separately keep track of `todo` for a `while(todo.Any())` loop. `FindCandidateSet()` Assumes that todo isn't empty, so we can't use that to query.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1668665030)
For each of the `CandidateFinder`s, would it be helpful to expose a `AllDone()` function? Seems slightly inconvenient for the caller to separately keep track of `todo` for a `while(todo.Any())` loop. `FindCandidateSet()` Assumes that todo isn't empty, so we can't use that to query.
💬 glozow commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1668716016)
Note to reviewers: pseudocode for the function when first introduced in 5661fcdb15244bc6d602379c294276c7ec686505 can be found [here](https://delvingbitcoin.org/t/how-to-linearize-your-cluster/303#h-21-searching-6)
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1668716016)
Note to reviewers: pseudocode for the function when first introduced in 5661fcdb15244bc6d602379c294276c7ec686505 can be found [here](https://delvingbitcoin.org/t/how-to-linearize-your-cluster/303#h-21-searching-6)
💬 glozow commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1668724062)
And I suppose it's not problematic if it is optimal but didn't claim to be so?
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1668724062)
And I suppose it's not problematic if it is optimal but didn't claim to be so?
📝 theStack opened a pull request: "rpc: doc: use "output script" terminology consistently in "asm"/"hex" results"
(https://github.com/bitcoin/bitcoin/pull/30408)
The wording "public key script" was likely chosen as a human-readable form of the technical term `scriptPubKey`, but it doesn't seem to be really widespread. Replace it by the more (probably most?) common term "output script" instead. Note that the argument for the `decodescript` RPC is not necessarily an output script (it could e.g. be also a redeem script), so in this case we just stay generic and use "script".
See also the draft BIP "Terminology for Transaction Components" (https://github.
...
(https://github.com/bitcoin/bitcoin/pull/30408)
The wording "public key script" was likely chosen as a human-readable form of the technical term `scriptPubKey`, but it doesn't seem to be really widespread. Replace it by the more (probably most?) common term "output script" instead. Note that the argument for the `decodescript` RPC is not necessarily an output script (it could e.g. be also a redeem script), so in this case we just stay generic and use "script".
See also the draft BIP "Terminology for Transaction Components" (https://github.
...
💬 maflcko commented on pull request "rpc: doc: use "output script" terminology consistently in "asm"/"hex" results":
(https://github.com/bitcoin/bitcoin/pull/30408#issuecomment-2214477162)
ACK 29eafd5733d77b3e8f3f3ab6cd65c61ac0e8536b
(https://github.com/bitcoin/bitcoin/pull/30408#issuecomment-2214477162)
ACK 29eafd5733d77b3e8f3f3ab6cd65c61ac0e8536b
🚀 ryanofsky merged a pull request: "kernel: De-globalize validation caches"
(https://github.com/bitcoin/bitcoin/pull/30141)
(https://github.com/bitcoin/bitcoin/pull/30141)
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1668941931)
I updated this to no longer clear flags.
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1668941931)
I updated this to no longer clear flags.
👍 BrandonOdiwuor approved a pull request: "test, assumeutxo: Remove resolved todo comments and add new test"
(https://github.com/bitcoin/bitcoin/pull/30403#pullrequestreview-2163834970)
Code Review ACK 29d19c414c9e138c99f8de84e398528db92ff07e
I will close https://github.com/bitcoin/bitcoin/pull/29681in favor of this
(https://github.com/bitcoin/bitcoin/pull/30403#pullrequestreview-2163834970)
Code Review ACK 29d19c414c9e138c99f8de84e398528db92ff07e
I will close https://github.com/bitcoin/bitcoin/pull/29681in favor of this
✅ BrandonOdiwuor closed a pull request: "test: loading assumeutxo snapshot start states"
(https://github.com/bitcoin/bitcoin/pull/29681)
(https://github.com/bitcoin/bitcoin/pull/29681)
💬 BrandonOdiwuor commented on pull request "test: loading assumeutxo snapshot start states":
(https://github.com/bitcoin/bitcoin/pull/29681#issuecomment-2214630023)
Closing PR in favor of https://github.com/bitcoin/bitcoin/pull/30403
(https://github.com/bitcoin/bitcoin/pull/29681#issuecomment-2214630023)
Closing PR in favor of https://github.com/bitcoin/bitcoin/pull/30403
💬 fjahr commented on pull request "test, assumeutxo: Remove resolved todo comments and add new test":
(https://github.com/bitcoin/bitcoin/pull/30403#issuecomment-2214654114)
Oh, sorry @BrandonOdiwuor , I missed https://github.com/bitcoin/bitcoin/pull/29681 😞
(https://github.com/bitcoin/bitcoin/pull/30403#issuecomment-2214654114)
Oh, sorry @BrandonOdiwuor , I missed https://github.com/bitcoin/bitcoin/pull/29681 😞
💬 ryanofsky commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-2214655675)
re: https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-2210605752
Thanks for the questions.
> I'm not sure what a good approach for reviewing this might be. How can I validate the changes without tests? Should I be running this as part of a parent pull request?
You could test this as part of #10102, and I should probably add some unit test code to [ipc_test.cpp](https://github.com/bitcoin/bitcoin/blob/master/src/test/ipc_test.cpp) to make sure new runtime code in [capnp/chain.cp
...
(https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-2214655675)
re: https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-2210605752
Thanks for the questions.
> I'm not sure what a good approach for reviewing this might be. How can I validate the changes without tests? Should I be running this as part of a parent pull request?
You could test this as part of #10102, and I should probably add some unit test code to [ipc_test.cpp](https://github.com/bitcoin/bitcoin/blob/master/src/test/ipc_test.cpp) to make sure new runtime code in [capnp/chain.cp
...
💬 darosior commented on pull request "PoC: fuzz chainstate and block managers":
(https://github.com/bitcoin/bitcoin/pull/29158#issuecomment-2214720674)
Rebased this, taking advantage of https://github.com/bitcoin/bitcoin/pull/28960. I've also been investigating alternative approaches.
I first tried to move from `fmemopen` toward the more flexible `memfd_create`. It avoided the need for some of the filesystem mocks (which were necessary before because you can't call `fileno` on a `FILE*` created with `fmemopen`). This allowed to drop one commit. Further, this removed the need from creating 128MiB in-memory blk files. This is in turn makes it
...
(https://github.com/bitcoin/bitcoin/pull/29158#issuecomment-2214720674)
Rebased this, taking advantage of https://github.com/bitcoin/bitcoin/pull/28960. I've also been investigating alternative approaches.
I first tried to move from `fmemopen` toward the more flexible `memfd_create`. It avoided the need for some of the filesystem mocks (which were necessary before because you can't call `fileno` on a `FILE*` created with `fmemopen`). This allowed to drop one commit. Further, this removed the need from creating 128MiB in-memory blk files. This is in turn makes it
...
📝 Sjors opened a pull request: "Introduce waitTipChanged() mining interface and replace RPCNotifyBlockChange"
(https://github.com/bitcoin/bitcoin/pull/30409)
This continues the work in #30200 so that a future Stratum v2 Template Provider (see #29432) can avoid accessing node internals. It needs to know when a new block arrives in order to push new templates to connected clients.
In order to ensure the new method works as intended, the `waitfornewblock`, `waitforblock` and `waitforblockheight` RPC methods are refactored to use it. This allows removing `RPCNotifyBlockChange`.
The second commit adds tests for the methods that are about to be refac
...
(https://github.com/bitcoin/bitcoin/pull/30409)
This continues the work in #30200 so that a future Stratum v2 Template Provider (see #29432) can avoid accessing node internals. It needs to know when a new block arrives in order to push new templates to connected clients.
In order to ensure the new method works as intended, the `waitfornewblock`, `waitforblock` and `waitforblockheight` RPC methods are refactored to use it. This allows removing `RPCNotifyBlockChange`.
The second commit adds tests for the methods that are about to be refac
...
💬 fjahr commented on issue "test: Write assumeutxo tests":
(https://github.com/bitcoin/bitcoin/issues/28648#issuecomment-2214748299)
I think this may be ready for closing since all remaining TODOs have been addressed IMO. There are three remaining PRs that still need to get merged but I think we can expect that nobody will start a new PR based on the TODOs comment anymore.
For reference, the remaining PRs are #30403, #30320, #29996. See also https://github.com/bitcoin/bitcoin/pull/30403#issuecomment-2213758446
(https://github.com/bitcoin/bitcoin/issues/28648#issuecomment-2214748299)
I think this may be ready for closing since all remaining TODOs have been addressed IMO. There are three remaining PRs that still need to get merged but I think we can expect that nobody will start a new PR based on the TODOs comment anymore.
For reference, the remaining PRs are #30403, #30320, #29996. See also https://github.com/bitcoin/bitcoin/pull/30403#issuecomment-2213758446
💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1669007098)
I'm assuming that it's only used for stronger invariants checks in the fuzz/test harness
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1669007098)
I'm assuming that it's only used for stronger invariants checks in the fuzz/test harness
💬 fjahr commented on issue "test: Write assumeutxo tests":
(https://github.com/bitcoin/bitcoin/issues/28648#issuecomment-2214756801)
Or rather, the problem is that the issue here is not specific and so none of the issues can close this issue because it's unclear which will be the last one to get merged. I will recommend in the PRs that they mention the issue here as related to so it's at least clear what is being worked on where.
(https://github.com/bitcoin/bitcoin/issues/28648#issuecomment-2214756801)
Or rather, the problem is that the issue here is not specific and so none of the issues can close this issue because it's unclear which will be the last one to get merged. I will recommend in the PRs that they mention the issue here as related to so it's at least clear what is being worked on where.