💬 TheCharlatan commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#issuecomment-3393634975)
> I expect it to have similar lookup performance, and since it wouldn'd require an additional Bitcoin Core index, I will prefer it over my original approach.
If the performance is similar, that sounds like the better course of action. I guess you'd still need the REST endpoint for retrieving the actual transaction?
(https://github.com/bitcoin/bitcoin/pull/32541#issuecomment-3393634975)
> I expect it to have similar lookup performance, and since it wouldn'd require an additional Bitcoin Core index, I will prefer it over my original approach.
If the performance is similar, that sounds like the better course of action. I guess you'd still need the REST endpoint for retrieving the actual transaction?
✅ fjahr closed a pull request: "refactor: Split multithreaded case out of CheckInputScripts"
(https://github.com/bitcoin/bitcoin/pull/32575)
(https://github.com/bitcoin/bitcoin/pull/32575)
💬 fjahr commented on pull request "refactor: Split multithreaded case out of CheckInputScripts":
(https://github.com/bitcoin/bitcoin/pull/32575#issuecomment-3393647663)
Silent merged conflict was resolved with the last push but the CI still fails with some problem fetching the qa-assets repo, seems to be a github issue since I also see it in master, retrying one more time with close/re-open to see if it works again now.
(https://github.com/bitcoin/bitcoin/pull/32575#issuecomment-3393647663)
Silent merged conflict was resolved with the last push but the CI still fails with some problem fetching the qa-assets repo, seems to be a github issue since I also see it in master, retrying one more time with close/re-open to see if it works again now.
📝 fjahr reopened a pull request: "refactor: Split multithreaded case out of CheckInputScripts"
(https://github.com/bitcoin/bitcoin/pull/32575)
This topic has been motivated by my work on batch validation and a related conversation just happened here: https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2098305199
`CheckInputScripts` currently only does what its name implies if there is no multithreading usage for validation. If there are worker threads available the function instead only creates the checks and puts them into a vector. Aside from some shared pre-checks the multithreaded code path is much simpler compared to the
...
(https://github.com/bitcoin/bitcoin/pull/32575)
This topic has been motivated by my work on batch validation and a related conversation just happened here: https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2098305199
`CheckInputScripts` currently only does what its name implies if there is no multithreading usage for validation. If there are worker threads available the function instead only creates the checks and puts them into a vector. Aside from some shared pre-checks the multithreaded code path is much simpler compared to the
...
💬 romanz commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#issuecomment-3393650305)
Yes - it will need way to fetch partial block data, e.g. https://github.com/romanz/bitcoin/commit/5a319571e728de009fb81a27d2bb86f8f8811d09
(https://github.com/bitcoin/bitcoin/pull/32541#issuecomment-3393650305)
Yes - it will need way to fetch partial block data, e.g. https://github.com/romanz/bitcoin/commit/5a319571e728de009fb81a27d2bb86f8f8811d09
🤔 enirox001 reviewed a pull request: "init: Fix Ctrl-C shutdown hangs during wait calls"
(https://github.com/bitcoin/bitcoin/pull/33511#pullrequestreview-3327587204)
Concept ACK c25a5e6
Replicated the issue; fix works. Good refactor in init and interfaces for interrupt support.
- regtest: RPC interrupts immediately on shutdown signal.
- testnet: RPC does not interrupt and waits for timeout.
Tested on Linux with Ctrl+C to trigger the signal interruption.
Please clarify testnet behavior.
(https://github.com/bitcoin/bitcoin/pull/33511#pullrequestreview-3327587204)
Concept ACK c25a5e6
Replicated the issue; fix works. Good refactor in init and interfaces for interrupt support.
- regtest: RPC interrupts immediately on shutdown signal.
- testnet: RPC does not interrupt and waits for timeout.
Tested on Linux with Ctrl+C to trigger the signal interruption.
Please clarify testnet behavior.
💬 fjahr commented on issue "Intermittent CI network issue downloading assets.json from GitHub":
(https://github.com/bitcoin/bitcoin/issues/33599#issuecomment-3393671508)
#32575 is now completely stuck on this, multiple jobs fail even after close/re-open. Seems like this has gotten worse if this was only hitting single jobs previously...
(https://github.com/bitcoin/bitcoin/issues/33599#issuecomment-3393671508)
#32575 is now completely stuck on this, multiple jobs fail even after close/re-open. Seems like this has gotten worse if this was only hitting single jobs previously...
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423143691)
Yes.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423143691)
Yes.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423144701)
Indeed unrelated.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423144701)
Indeed unrelated.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423148286)
Without `m_quality` and `m_setindex` private, I think this would be confusing. They feel like they belong together.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423148286)
Without `m_quality` and `m_setindex` private, I think this would be confusing. They feel like they belong together.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423151243)
Unrelated, indeed.
Also, I wouldn't be surprised if the cost of a CTZ to find the next set bit, and the less predictable access pattern, is greater than the cost of performing two ANDs on unused positions. In either case, I don't think this matters.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423151243)
Unrelated, indeed.
Also, I wouldn't be surprised if the cost of a CTZ to find the next set bit, and the less predictable access pattern, is greater than the cost of performing two ANDs on unused positions. In either case, I don't think this matters.
💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2423151639)
Thanks for the review @kannapoix
mocktime is no longer needed in this test, as it passes without it. The block generation and stats comparison occur within the same run, ensuring consistency. Remove the mocktime lines entirely for simplicity.
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2423151639)
Thanks for the review @kannapoix
mocktime is no longer needed in this test, as it passes without it. The block generation and stats comparison occur within the same run, ensuring consistency. Remove the mocktime lines entirely for simplicity.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423153015)
I think of the `MIN_INTENDED_TX_COUNT` and `MAX_TX_COUNT` as the best emulation of having `static` versions of `MinIntendedTxCount()` and `MaxTxCount()` (because in C++, class methods cannot be virtual, so they can't be made part of the `Cluster` formal interface). So we're stuck with two versions: a polymorphic instance member function, and a non-polymorphic class member variable.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423153015)
I think of the `MIN_INTENDED_TX_COUNT` and `MAX_TX_COUNT` as the best emulation of having `static` versions of `MinIntendedTxCount()` and `MaxTxCount()` (because in C++, class methods cannot be virtual, so they can't be made part of the `Cluster` formal interface). So we're stuck with two versions: a polymorphic instance member function, and a non-polymorphic class member variable.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423159000)
The first two here are sort of expected, as they're only in instantiations with the MultiIntBitSet as set type. There is another fuzz test that verifies the behavior of that instance as correct, and specifically, equivalent to IntBitSet. At higher levels `IntBitSet` is the only one used in actual production code, while `MultiIntBitSet` is only used in tests (for example, in the txgraph fuzz simulation).
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423159000)
The first two here are sort of expected, as they're only in instantiations with the MultiIntBitSet as set type. There is another fuzz test that verifies the behavior of that instance as correct, and specifically, equivalent to IntBitSet. At higher levels `IntBitSet` is the only one used in actual production code, while `MultiIntBitSet` is only used in tests (for example, in the txgraph fuzz simulation).
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423160407)
I don't think so; it needs a different approach.
The current approach copies in linearization order. A faster approach (which would need additional logic in `DepGraph` too) would return the DepGraphIndex order of transactions, and applies offsets.
I'll consider this in my (not yet pushed) updates to #32545, which introduce a substantial change: allowing linearizations to be non-topological. That currently requires merging in two phases (first copy all transactions, then all dependencies).
...
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423160407)
I don't think so; it needs a different approach.
The current approach copies in linearization order. A faster approach (which would need additional logic in `DepGraph` too) would return the DepGraphIndex order of transactions, and applies offsets.
I'll consider this in my (not yet pushed) updates to #32545, which introduce a substantial change: allowing linearizations to be non-topological. That currently requires merging in two phases (first copy all transactions, then all dependencies).
...
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423160627)
Ok, resolving.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423160627)
Ok, resolving.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423166549)
Done.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423166549)
Done.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423166682)
Added a comment in the txgraph.h definition of the function.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423166682)
Added a comment in the txgraph.h definition of the function.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423166756)
Done.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423166756)
Done.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423166844)
Done.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423166844)
Done.