💬 fjahr commented on pull request "[EXPERIMENTAL] Schnorr batch verification for blocks":
(https://github.com/bitcoin/bitcoin/pull/29491#discussion_r2009282526)
> 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. We need CCheckQueue::Complete to set a flag that no more checks will be added, and wake all threads again. The threads will all verify their batches once the global queue is empty. We would need to reset the flag after Complete.
Yes, one batch per thread is the approach I mentioned above as well as preferred, if
...
(https://github.com/bitcoin/bitcoin/pull/29491#discussion_r2009282526)
> 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. We need CCheckQueue::Complete to set a flag that no more checks will be added, and wake all threads again. The threads will all verify their batches once the global queue is empty. We would need to reset the flag after Complete.
Yes, one batch per thread is the approach I mentioned above as well as preferred, if
...
📝 Elmarinero1983 opened a pull request: "5e8bc97cdc6dd8d7be10f0ac8c1b46d2c2fd1547"
(https://github.com/bitcoin/bitcoin/pull/32127)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/32127)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
✅ fanquake closed a pull request: "5e8bc97cdc6dd8d7be10f0ac8c1b46d2c2fd1547"
(https://github.com/bitcoin/bitcoin/pull/32127)
(https://github.com/bitcoin/bitcoin/pull/32127)
💬 frankomosh commented on issue "29.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/32026#issuecomment-2746971852)
> This issue is to discuss the [29.0 Release Candidate Testing Guide](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/29.0-Release-Candidate-Testing-Guide). If you have any feedback on the document, please leave a comment here.
>
> Note: This is for feedback on the document, not on Bitcoin Core or on the 29.0 changes.
>
> Thank you for taking a look at the guide and leaving your feedback.
>
> ps. The initial page was co-authored-by: [@arejula27](https://github.com/arejula27) [@musaHaruna
...
(https://github.com/bitcoin/bitcoin/issues/32026#issuecomment-2746971852)
> This issue is to discuss the [29.0 Release Candidate Testing Guide](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/29.0-Release-Candidate-Testing-Guide). If you have any feedback on the document, please leave a comment here.
>
> Note: This is for feedback on the document, not on Bitcoin Core or on the 29.0 changes.
>
> Thank you for taking a look at the guide and leaving your feedback.
>
> ps. The initial page was co-authored-by: [@arejula27](https://github.com/arejula27) [@musaHaruna
...
📝 martinus opened a pull request: "Draft: CCoinMap Experiments"
(https://github.com/bitcoin/bitcoin/pull/32128)
This is a draft PR to show various possible optimizations for `CCoinsMap`. In my benchmark, all these changes lead to a statistically significant speed improvement for `-reindex-chainstate`.
```
Benchmark 1: ./build/bin/bitcoind -stopatheight=600000 -dbcache=5000 -printtoconsole=0 -reindex-chainstate -noconnect -connect=192.168.8.118 (COMMIT = ac188b573c8)
Time (mean ± σ): 2089.253 s ± 23.737 s [User: 2110.111 s, System: 299.197 s]
Range (min … max): 2062.751 s … 2108.561 s
...
(https://github.com/bitcoin/bitcoin/pull/32128)
This is a draft PR to show various possible optimizations for `CCoinsMap`. In my benchmark, all these changes lead to a statistically significant speed improvement for `-reindex-chainstate`.
```
Benchmark 1: ./build/bin/bitcoind -stopatheight=600000 -dbcache=5000 -printtoconsole=0 -reindex-chainstate -noconnect -connect=192.168.8.118 (COMMIT = ac188b573c8)
Time (mean ± σ): 2089.253 s ± 23.737 s [User: 2110.111 s, System: 299.197 s]
Range (min … max): 2062.751 s … 2108.561 s
...
💬 maflcko commented on pull request "signet: omit commitment for some trivial challenges":
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r2009586472)
https://cirrus-ci.com/task/5609902313308160?logs=ci#L5104
```
[20:25:29.234] test 2025-03-24T00:25:28.624000Z TestFramework (ERROR): Called Process failed with 'b'''
[20:25:29.234] Traceback (most recent call last):
[20:25:29.234] File "/ci_container_base/test/functional/test_framework/test_framework.py", line 178, in main
[20:25:29.234] self.run_test()
[20:25:29.234]
...
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r2009586472)
https://cirrus-ci.com/task/5609902313308160?logs=ci#L5104
```
[20:25:29.234] test 2025-03-24T00:25:28.624000Z TestFramework (ERROR): Called Process failed with 'b'''
[20:25:29.234] Traceback (most recent call last):
[20:25:29.234] File "/ci_container_base/test/functional/test_framework/test_framework.py", line 178, in main
[20:25:29.234] self.run_test()
[20:25:29.234]
...
💬 maflcko commented on issue "Wallet passpharse":
(https://github.com/bitcoin/bitcoin/issues/31852#issuecomment-2747136359)
Now that 29.0 is in the release phase, you could also try that: https://bitcoincore.org/bin/bitcoin-core-29.0/test.rc2/ (It is the rc, so only use it for testing and after making a backup, etc ...)
(https://github.com/bitcoin/bitcoin/issues/31852#issuecomment-2747136359)
Now that 29.0 is in the release phase, you could also try that: https://bitcoincore.org/bin/bitcoin-core-29.0/test.rc2/ (It is the rc, so only use it for testing and after making a backup, etc ...)
💬 Sjors commented on pull request "signet: omit commitment for some trivial challenges":
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r2009614780)
I can reproduce this once I rebase on the latest master. I'll investigate.
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r2009614780)
I can reproduce this once I rebase on the latest master. I'll investigate.
💬 maflcko commented on pull request "fuzz: Fix off-by-one in package_rbf target":
(https://github.com/bitcoin/bitcoin/pull/32122#issuecomment-2747190493)
> why not just modify `initialize_package_rbf` to use this statement
Why would that be better? Once an off-by-two is added in a future patch, the fuzz target once again will crash, whereas with the patch in this pull, it will work fine?
(https://github.com/bitcoin/bitcoin/pull/32122#issuecomment-2747190493)
> why not just modify `initialize_package_rbf` to use this statement
Why would that be better? Once an off-by-two is added in a future patch, the fuzz target once again will crash, whereas with the patch in this pull, it will work fine?
💬 Sjors commented on pull request "signet: omit commitment for some trivial challenges":
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r2009667758)
It seems to be caused by #31866, because the test passes when I revert it.
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r2009667758)
It seems to be caused by #31866, because the test passes when I revert it.
💬 maflcko commented on pull request "signet: omit commitment for some trivial challenges":
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r2009685207)
You'll have to use `node.binaries.rpc_argv()` instead here
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r2009685207)
You'll have to use `node.binaries.rpc_argv()` instead here
💬 Sjors commented on pull request "signet: omit commitment for some trivial challenges":
(https://github.com/bitcoin/bitcoin/pull/29032#issuecomment-2747275625)
Rebased after #31866 to use `--cli={shlex.join(rpc_argv)}` in `mine_block_manual`.
(https://github.com/bitcoin/bitcoin/pull/29032#issuecomment-2747275625)
Rebased after #31866 to use `--cli={shlex.join(rpc_argv)}` in `mine_block_manual`.
💬 maflcko commented on pull request "fuzz: enable running fuzz test cases in Debug mode":
(https://github.com/bitcoin/bitcoin/pull/32113#discussion_r2009712764)
Is there a use-case for FUZZ_NONDETERMINISM? IIRC the goal of this change was to achieve fuzz determinism, so adding non-determinism could be left for a follow-up? I'd even argue that it may be better to not add at all, because it is such an edge case that no one will be using it. However, it existing, implies that reviewers will have to take it into account every time, which doesn't seem worth it.
If you remove it, you can also move the `assert(EnableFuzzDeterminism());` before `Assert(!g_te
...
(https://github.com/bitcoin/bitcoin/pull/32113#discussion_r2009712764)
Is there a use-case for FUZZ_NONDETERMINISM? IIRC the goal of this change was to achieve fuzz determinism, so adding non-determinism could be left for a follow-up? I'd even argue that it may be better to not add at all, because it is such an edge case that no one will be using it. However, it existing, implies that reviewers will have to take it into account every time, which doesn't seem worth it.
If you remove it, you can also move the `assert(EnableFuzzDeterminism());` before `Assert(!g_te
...
💬 maflcko commented on issue "bitcoind crash with corrupt wallet.dat":
(https://github.com/bitcoin/bitcoin/issues/32124#issuecomment-2747328830)
> Not sure if it is relevant to [#32111](https://github.com/bitcoin/bitcoin/issues/32111).
32111 is about migrating (corrupt) bdb wallets (likely obtained from fuzzing), with exact steps to reproduce and the crash being due to `Assertion m_wallet_flags == 0 failed`. Whereas this issue is about loading (?) a corrupt (?) sqlite wallet, without steps to reproduce. Also, it is unclear how the two sqlite errors interact and if the second one is due to another dangling process on the system, or not.
...
(https://github.com/bitcoin/bitcoin/issues/32124#issuecomment-2747328830)
> Not sure if it is relevant to [#32111](https://github.com/bitcoin/bitcoin/issues/32111).
32111 is about migrating (corrupt) bdb wallets (likely obtained from fuzzing), with exact steps to reproduce and the crash being due to `Assertion m_wallet_flags == 0 failed`. Whereas this issue is about loading (?) a corrupt (?) sqlite wallet, without steps to reproduce. Also, it is unclear how the two sqlite errors interact and if the second one is due to another dangling process on the system, or not.
...
🤔 rkrux reviewed a pull request: "contrib: refactor: dedup deserialization routines in utxo-to-sqlite script"
(https://github.com/bitcoin/bitcoin/pull/32116#pullrequestreview-2709590637)
utACK f535b2fe63255175f7a35882599f9b6b83ac25f1
I did realise there'd be some duplication when I previously supported adding and using these deserialisation functions in https://github.com/bitcoin/bitcoin/pull/31907#discussion_r1965270838, good that you found a way to get rid of the recently introduced duplication.
(https://github.com/bitcoin/bitcoin/pull/32116#pullrequestreview-2709590637)
utACK f535b2fe63255175f7a35882599f9b6b83ac25f1
I did realise there'd be some duplication when I previously supported adding and using these deserialisation functions in https://github.com/bitcoin/bitcoin/pull/31907#discussion_r1965270838, good that you found a way to get rid of the recently introduced duplication.
💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r2009729335)
I improved the commit message.
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r2009729335)
I improved the commit message.
💬 Sjors commented on pull request "rpc: add optional blockhash to waitfornewblock, unhide wait methods in help":
(https://github.com/bitcoin/bitcoin/pull/30635#issuecomment-2747341913)
My latest change to #31785 only changed the commit message, so I'm not rebasing this yet.
(https://github.com/bitcoin/bitcoin/pull/30635#issuecomment-2747341913)
My latest change to #31785 only changed the commit message, so I'm not rebasing this yet.
💬 hodlinator commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2747360632)
### unique_ptr
Here is a branch implementing another possible variant, using `unique_ptr` as suggested by @purpleKarrot in https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2711842250:
https://github.com/bitcoin/bitcoin/compare/master...hodlinator:bitcoin:pr/32015_unique_ptr
#### Pros
- It keeps the `CNode` ownership/lifetime under control of `CConnman::m_nodes`/`m_nodes_disconnected`. This can be considered an advancement over the unclear ownership/lifetime of `shared_ptr` ([
...
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2747360632)
### unique_ptr
Here is a branch implementing another possible variant, using `unique_ptr` as suggested by @purpleKarrot in https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2711842250:
https://github.com/bitcoin/bitcoin/compare/master...hodlinator:bitcoin:pr/32015_unique_ptr
#### Pros
- It keeps the `CNode` ownership/lifetime under control of `CConnman::m_nodes`/`m_nodes_disconnected`. This can be considered an advancement over the unclear ownership/lifetime of `shared_ptr` ([
...
💬 maflcko commented on issue "Failed transactions on importing mempool":
(https://github.com/bitcoin/bitcoin/issues/32125#issuecomment-2747360730)
> The nodes I test are running on my local network so I guess all the transactions are importable, but the 10 failed could be 5 packages of 2.
Can you add exact steps to reproduce, please?
The mempool acceptance depends on the chain state (the best block hash of your node at the time of the mempool import), as well as the mempool file contents.
Without any exact info, my guess would be that the 10 failures are due to transactions already confirmed (either themselves or a replacement of themse
...
(https://github.com/bitcoin/bitcoin/issues/32125#issuecomment-2747360730)
> The nodes I test are running on my local network so I guess all the transactions are importable, but the 10 failed could be 5 packages of 2.
Can you add exact steps to reproduce, please?
The mempool acceptance depends on the chain state (the best block hash of your node at the time of the mempool import), as well as the mempool file contents.
Without any exact info, my guess would be that the 10 failures are due to transactions already confirmed (either themselves or a replacement of themse
...
💬 Sjors commented on pull request "Rust tool to import bip39 mnemonic":
(https://github.com/bitcoin/bitcoin/pull/32115#discussion_r2009752779)
The mnemonic. Some wallets let you see the mnemonic, others don't (e.g. Ledger only shows it on first use, Bitkey doesn't show it all). But we can't show it, because the master hd key is a hash of the seed, so there's no way back.
If the original wallet uses different derivation paths then Bitcoin Core won't find funds, but they're not lost. Using the same key material between two wallets is a bad idea in general, so we could advice against that.
The docs should probably also mention that
...
(https://github.com/bitcoin/bitcoin/pull/32115#discussion_r2009752779)
The mnemonic. Some wallets let you see the mnemonic, others don't (e.g. Ledger only shows it on first use, Bitkey doesn't show it all). But we can't show it, because the master hd key is a hash of the seed, so there's no way back.
If the original wallet uses different derivation paths then Bitcoin Core won't find funds, but they're not lost. Using the same key material between two wallets is a bad idea in general, so we could advice against that.
The docs should probably also mention that
...