💬 tdb3 commented on pull request "doc: add guidance for RPC to developer notes":
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r2004449224)
Updated. Thanks.
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r2004449224)
Updated. Thanks.
💬 fjahr commented on pull request "[EXPERIMENTAL] Schnorr batch verification for blocks":
(https://github.com/bitcoin/bitcoin/pull/29491#discussion_r2004525996)
You're right, this wasn't a complete solution. I have improved this now by using buckets of signatures for each thread which are all added and only then verified by the master thread at the end. This could be further improved if we could add the signatures to a batch per thread right away but this would require that we could merge batch objects which the secp api currently doesn't make possible. I'll add that to my wish list because I think it should be even better than the current approach but
...
(https://github.com/bitcoin/bitcoin/pull/29491#discussion_r2004525996)
You're right, this wasn't a complete solution. I have improved this now by using buckets of signatures for each thread which are all added and only then verified by the master thread at the end. This could be further improved if we could add the signatures to a batch per thread right away but this would require that we could merge batch objects which the secp api currently doesn't make possible. I'll add that to my wish list because I think it should be even better than the current approach but
...
🤔 furszy reviewed a pull request: "test: assumeutxo: add missing tests in wallet_assumeutxo.py"
(https://github.com/bitcoin/bitcoin/pull/30455#pullrequestreview-2700676883)
Could you decouple this test into its own isolated function?
The current monolithic approach makes it harder to review, even when the changes seem simple.
See for example how [wallet_migration.py](https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_migration.py) is structured.
(https://github.com/bitcoin/bitcoin/pull/30455#pullrequestreview-2700676883)
Could you decouple this test into its own isolated function?
The current monolithic approach makes it harder to review, even when the changes seem simple.
See for example how [wallet_migration.py](https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_migration.py) is structured.
💬 jimhashhq commented on pull request "Multiprocess bitcoin":
(https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-2738635146)
Good news -- with your rebase of #19461, I am now able to try `-ipcconnect`, thank you.
In this multiprocess taxonomy, for separation-of-concerns and or separation-of-roles support, I was expecting that the default would be for the `node` not to start a `wallet` at all, instead having the `gui` start a `wallet` in addition to a `node`.
I vote somebody create some sort of `MULTIPROCESS` label for issue tracking if that makes sense.
Describing `CMAKE_PREFIX_PATH` for external modules is p
...
(https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-2738635146)
Good news -- with your rebase of #19461, I am now able to try `-ipcconnect`, thank you.
In this multiprocess taxonomy, for separation-of-concerns and or separation-of-roles support, I was expecting that the default would be for the `node` not to start a `wallet` at all, instead having the `gui` start a `wallet` in addition to a `node`.
I vote somebody create some sort of `MULTIPROCESS` label for issue tracking if that makes sense.
Describing `CMAKE_PREFIX_PATH` for external modules is p
...
💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004544338)
Maybe it would be helpful to explain in the header that AddTransaction() creates a Ref, which you're then expected to assign/move into your own container, which may be a subclass containing even more info about the tx, and then that serves as a permanent handle, even as the txgraph gets rearranged internally and that it's deleted from the graph either with RemoveTransaction explicitly or implicitly by destructing the Ref? Essentially expanding the "Data type used to reference transactions within
...
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004544338)
Maybe it would be helpful to explain in the header that AddTransaction() creates a Ref, which you're then expected to assign/move into your own container, which may be a subclass containing even more info about the tx, and then that serves as a permanent handle, even as the txgraph gets rearranged internally and that it's deleted from the graph either with RemoveTransaction explicitly or implicitly by destructing the Ref? Essentially expanding the "Data type used to reference transactions within
...
💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004553119)
Sounds like this is something that can be reconsidered when we get a PR that introduces variant Clusters then. Mark as resolved for now?
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004553119)
Sounds like this is something that can be reconsidered when we get a PR that introduces variant Clusters then. Mark as resolved for now?
💬 andrewtoth commented on pull request "[EXPERIMENTAL] Schnorr batch verification for blocks":
(https://github.com/bitcoin/bitcoin/pull/29491#discussion_r2004559393)
I think @Eunovo's approach is closer to what we want. Instead of a bucket per thread, have a batch per thread. However, instead of batch verifying after each `vChecks` batch, batch verify after the queue of checks is empty for the block.
This approach will not be faster even with pippinger algo. The max speedup is closer to 2x, but multi threaded is 16x faster.
(https://github.com/bitcoin/bitcoin/pull/29491#discussion_r2004559393)
I think @Eunovo's approach is closer to what we want. Instead of a bucket per thread, have a batch per thread. However, instead of batch verifying after each `vChecks` batch, batch verify after the queue of checks is empty for the block.
This approach will not be faster even with pippinger algo. The max speedup is closer to 2x, but multi threaded is 16x faster.
🚀 fanquake merged a pull request: "test: replace assert with assert_equal and assert_greater_than"
(https://github.com/bitcoin/bitcoin/pull/32091)
(https://github.com/bitcoin/bitcoin/pull/32091)
🚀 fanquake merged a pull request: "leveldb: pull upstream C++23 changes"
(https://github.com/bitcoin/bitcoin/pull/31766)
(https://github.com/bitcoin/bitcoin/pull/31766)
🚀 fanquake merged a pull request: "fuzz: Speed up *_package_eval fuzz targets a bit"
(https://github.com/bitcoin/bitcoin/pull/31457)
(https://github.com/bitcoin/bitcoin/pull/31457)
🚀 fanquake merged a pull request: "refactor: Use std::span over Span"
(https://github.com/bitcoin/bitcoin/pull/31519)
(https://github.com/bitcoin/bitcoin/pull/31519)
⚠️ User087 opened an issue: "Add a `indexesdir` option to hold the indexes directory"
(https://github.com/bitcoin/bitcoin/issues/32099)
### Please describe the feature you'd like to see added.
An `indexesdir` option (or whatever you want to call it) to specify an alternative directory to datadir to hold the 'indexes' subdirectory, like the `blocksdir` option for the 'blocks' subdirectory.
### Is your feature related to a problem, if so please describe it.
Like the 'blocks' directory, the 'indexes' directory can grow to a significant size (perhaps not as large as 'blocks' but still significantly large), making it desirable to
...
(https://github.com/bitcoin/bitcoin/issues/32099)
### Please describe the feature you'd like to see added.
An `indexesdir` option (or whatever you want to call it) to specify an alternative directory to datadir to hold the 'indexes' subdirectory, like the `blocksdir` option for the 'blocks' subdirectory.
### Is your feature related to a problem, if so please describe it.
Like the 'blocks' directory, the 'indexes' directory can grow to a significant size (perhaps not as large as 'blocks' but still significantly large), making it desirable to
...
💬 davidgumberg commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2739341760)
tested reACK https://github.com/bitcoin/bitcoin/commit/25b56fd9b469f8e5d36f0132c3b79a5214e3372a
Tested that this branch catches both unit and functional test failures.
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2739341760)
tested reACK https://github.com/bitcoin/bitcoin/commit/25b56fd9b469f8e5d36f0132c3b79a5214e3372a
Tested that this branch catches both unit and functional test failures.
💬 stratospher commented on pull request "net: Block v2->v1 transport downgrade if !fNetworkActive":
(https://github.com/bitcoin/bitcoin/pull/32073#issuecomment-2739349521)
ACK 6869fb4. I've reviewed the code but don't have strong preference for this branch vs master since only functional change is just a single log not being printed in a low probability scenario (we happen to be attempting v2 connection when P2P network activity is turned off).
(https://github.com/bitcoin/bitcoin/pull/32073#issuecomment-2739349521)
ACK 6869fb4. I've reviewed the code but don't have strong preference for this branch vs master since only functional change is just a single log not being printed in a low probability scenario (we happen to be attempting v2 connection when P2P network activity is turned off).
💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004931183)
If I'm understanding right, this seem like it makes `IsOversized()` largely unfixable by the client -- so the only reasonable strategy I can see is to do:
```c++
StartStaging();
// change transactions
if (IsOversized() || otherwise_undesirable()) {
AbortStaging();
} else {
CommitStaging();`
}
```
That's a very reasonable strategy of course, so this isn't a complaint! Just that if so, it seems like it should be documented a bit more clearly? I wo
...
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004931183)
If I'm understanding right, this seem like it makes `IsOversized()` largely unfixable by the client -- so the only reasonable strategy I can see is to do:
```c++
StartStaging();
// change transactions
if (IsOversized() || otherwise_undesirable()) {
AbortStaging();
} else {
CommitStaging();`
}
```
That's a very reasonable strategy of course, so this isn't a complaint! Just that if so, it seems like it should be documented a bit more clearly? I wo
...
💬 laanwj commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2739398089)
My guix output matches @hebasto's
<details>
<summary>hashes</summary>
```
find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
7638ac47fcd9ea8a4e2bbb265876aaeba0d8fe774f5bcc884753331d6ffe3429 guix-build-94967c353ed8/output/aarch64-linux-gnu/SHA256SUMS.part
3d7ff7c2c1ad1608946452645d9047ceef0e77f589098c94b5d628476ee5192d guix-build-94967c353ed8/output/aarch64-linux-gnu/bitcoin-94967c353ed8-aarch64-linux-gnu-debug.tar.gz
...
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2739398089)
My guix output matches @hebasto's
<details>
<summary>hashes</summary>
```
find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
7638ac47fcd9ea8a4e2bbb265876aaeba0d8fe774f5bcc884753331d6ffe3429 guix-build-94967c353ed8/output/aarch64-linux-gnu/SHA256SUMS.part
3d7ff7c2c1ad1608946452645d9047ceef0e77f589098c94b5d628476ee5192d guix-build-94967c353ed8/output/aarch64-linux-gnu/bitcoin-94967c353ed8-aarch64-linux-gnu-debug.tar.gz
...
💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2004835358)
In 9b8fa3cd2e0d5851355352e1de37c76bd3821e96:
It's not clear why this change is needed, and I think it's going in the wrong direction; `gcc` should not be getting hardcoded here (#30206).
The commit message also seems to be a mashup of things that aren't necessarily related to Qt, and makes out that this is macOS related (even though the change is being applied to all platforms). It also doesn't explain the actual issue, just that apparently the current code is overkill/undesirable. What's th
...
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2004835358)
In 9b8fa3cd2e0d5851355352e1de37c76bd3821e96:
It's not clear why this change is needed, and I think it's going in the wrong direction; `gcc` should not be getting hardcoded here (#30206).
The commit message also seems to be a mashup of things that aren't necessarily related to Qt, and makes out that this is macOS related (even though the change is being applied to all platforms). It also doesn't explain the actual issue, just that apparently the current code is overkill/undesirable. What's th
...
💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2004895158)
In 428942fdd64b477eef809c2b2fb0906472f2449c:
Why is `-pkg-config` usage being reintroduced here?
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2004895158)
In 428942fdd64b477eef809c2b2fb0906472f2449c:
Why is `-pkg-config` usage being reintroduced here?
💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004952081)
see also the TODO item
> * Ability to inspect the would-be-constructed clusters (so they can be "fixed" if they violate policy rules, as opposed to just accepted/rejected, before applying - this is needed for reorgs which are not optional).
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2004952081)
see also the TODO item
> * Ability to inspect the would-be-constructed clusters (so they can be "fixed" if they violate policy rules, as opposed to just accepted/rejected, before applying - this is needed for reorgs which are not optional).
💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2739415197)
(conflicts with just merged #31519, Span to std::span)
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2739415197)
(conflicts with just merged #31519, Span to std::span)