💬 stringintech commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2004380489)
Can the `m_best_header` update logic be merged into the while loop that handles the `nStatus` updates? Not sure if it is necessarily better; but I was thinking of something like:
Adding this just before the while loop:
```c++
const bool best_header_needs_update = m_chainman.m_best_header->GetAncestor(invalid_walk_tip->nHeight) == invalid_walk_tip;
if (best_header_needs_update) {
m_chainman.m_best_header = invalid_walk_tip->pprev;
}
```
And then here:
```suggestion
if
...
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2004380489)
Can the `m_best_header` update logic be merged into the while loop that handles the `nStatus` updates? Not sure if it is necessarily better; but I was thinking of something like:
Adding this just before the while loop:
```c++
const bool best_header_needs_update = m_chainman.m_best_header->GetAncestor(invalid_walk_tip->nHeight) == invalid_walk_tip;
if (best_header_needs_update) {
m_chainman.m_best_header = invalid_walk_tip->pprev;
}
```
And then here:
```suggestion
if
...
💬 stringintech commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2004381740)
Shouldn't we mark the candidate dirty after this?
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2004381740)
Shouldn't we mark the candidate dirty after this?
💬 stringintech commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2004383630)
Small typo at the end of the comment
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2004383630)
Small typo at the end of the comment
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2738349197)
> It could enable other approaches to building bindings and using kernel code in external projects.
I have done some experimentation with using c++ bindings directly.
> [SWIG](https://www.swig.org/)
I tried creating python bindings through swig without looping through the C bindings and I could not get it to work within a reasonable amount of time. While likely a skill issue, it does seem to be struggling with some of our c++20 features and heavily templated code.
> [pybind](https:
...
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2738349197)
> It could enable other approaches to building bindings and using kernel code in external projects.
I have done some experimentation with using c++ bindings directly.
> [SWIG](https://www.swig.org/)
I tried creating python bindings through swig without looping through the C bindings and I could not get it to work within a reasonable amount of time. While likely a skill issue, it does seem to be struggling with some of our c++20 features and heavily templated code.
> [pybind](https:
...
💬 tdb3 commented on pull request "doc: add guidance for RPC to developer notes":
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r2004449128)
Updated. Thanks.
(https://github.com/bitcoin/bitcoin/pull/30142#discussion_r2004449128)
Updated. Thanks.
💬 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
...