🤔 maflcko reviewed a pull request: "net: make m_mempool optional in PeerManager"
(https://github.com/bitcoin/bitcoin/pull/33029#pullrequestreview-3042100735)
I think the changes are conceptually nice, but hard to see a user-visible benefit from this alone. Combined with the difficulty to review this (in its current state) around nullptr deref, it will probably be a hard sell.
(https://github.com/bitcoin/bitcoin/pull/33029#pullrequestreview-3042100735)
I think the changes are conceptually nice, but hard to see a user-visible benefit from this alone. Combined with the difficulty to review this (in its current state) around nullptr deref, it will probably be a hard sell.
💬 maflcko commented on pull request "net: make m_mempool optional in PeerManager":
(https://github.com/bitcoin/bitcoin/pull/33029#discussion_r2221894883)
I don't understand why you removed the assert (https://github.com/bitcoin/bitcoin/pull/26247/files#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R1161)
`PartiallyDownloadedBlock` can't deal with nullptr and dereferencing a nullptr will be UB and will usually lead to a crash.
(https://github.com/bitcoin/bitcoin/pull/33029#discussion_r2221894883)
I don't understand why you removed the assert (https://github.com/bitcoin/bitcoin/pull/26247/files#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R1161)
`PartiallyDownloadedBlock` can't deal with nullptr and dereferencing a nullptr will be UB and will usually lead to a crash.
💬 maflcko commented on pull request "docs: adds correct updated documentation links":
(https://github.com/bitcoin/bitcoin/pull/32699#issuecomment-3101889213)
> all good now
no, it is not. This can be seen if you take a look at the changes yourself.
Also, it would have to be squashed, as already explained above as well.
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
(https://github.com/bitcoin/bitcoin/pull/32699#issuecomment-3101889213)
> all good now
no, it is not. This can be seen if you take a look at the changes yourself.
Also, it would have to be squashed, as already explained above as well.
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
💬 maflcko commented on pull request "test: Add functional tests for blockreconstructionextratxn parameter":
(https://github.com/bitcoin/bitcoin/pull/33023#issuecomment-3101914384)
Could turn into draft while CI is failing?
(https://github.com/bitcoin/bitcoin/pull/33023#issuecomment-3101914384)
Could turn into draft while CI is failing?
💬 rkrux commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2221936637)
> However, the descriptor unit tests often call ToPrivateString() on watch-only descriptors
Yeah, I don't know why this was done.
> To add this assertion, we will have to remove those checks, and I haven't seen the advantage in doing that.
Agree, doesn't need to be done right now.
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2221936637)
> However, the descriptor unit tests often call ToPrivateString() on watch-only descriptors
Yeah, I don't know why this was done.
> To add this assertion, we will have to remove those checks, and I haven't seen the advantage in doing that.
Agree, doesn't need to be done right now.
💬 maflcko commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2221958115)
less relevant here, but below you are removing the assert on the `CInv&`. Seems fine to at least restore this as an Assume?
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2221958115)
less relevant here, but below you are removing the assert on the `CInv&`. Seems fine to at least restore this as an Assume?
⚠️ fanquake opened an issue: "SegFault in QSortFilterProxyModelPrivate::build_source_to_proxy_mapping"
(https://github.com/bitcoin-core/gui/issues/880)
Moved from https://github.com/bitcoin/bitcoin/issues/32957.
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
I was loading a bunch of old wallets sequentially using "bitcoin-cli loadwallet" in a loop. bitcoin-qt crashed shortly after finishing loading one wallet and starting to load the next.
The debug log shows this at the end:
2025-07-13T13:06:26Z init message: Rescanning…
2025-07-13T13:06:26Z [wallet23] Rescanning last 3130
...
(https://github.com/bitcoin-core/gui/issues/880)
Moved from https://github.com/bitcoin/bitcoin/issues/32957.
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
I was loading a bunch of old wallets sequentially using "bitcoin-cli loadwallet" in a loop. bitcoin-qt crashed shortly after finishing loading one wallet and starting to load the next.
The debug log shows this at the end:
2025-07-13T13:06:26Z init message: Rescanning…
2025-07-13T13:06:26Z [wallet23] Rescanning last 3130
...
💬 fanquake commented on issue "SegFault in QSortFilterProxyModelPrivate::build_source_to_proxy_mapping":
(https://github.com/bitcoin-core/gui/issues/880#issuecomment-3101966993)
I just had the same crash happen again. This time very shortly after starting bitcoin-qt, while it was still loading the mempool from disk. I ran the 'loadwallet' loop again, and it crashed on the 2nd wallet in the loop. There's one wallet loaded from `bitcoin.conf` where it says "wallet=wallet1". I waited for that to finish loading before starting the "bitcoin-cli loadwallet $w" loop.
Here's the debug.log:
2025-07-13T22:25:10Z Leaving InitialBlockDownload (latching to false)
2025-07-1
...
(https://github.com/bitcoin-core/gui/issues/880#issuecomment-3101966993)
I just had the same crash happen again. This time very shortly after starting bitcoin-qt, while it was still loading the mempool from disk. I ran the 'loadwallet' loop again, and it crashed on the 2nd wallet in the loop. There's one wallet loaded from `bitcoin.conf` where it says "wallet=wallet1". I waited for that to finish loading before starting the "bitcoin-cli loadwallet $w" loop.
Here's the debug.log:
2025-07-13T22:25:10Z Leaving InitialBlockDownload (latching to false)
2025-07-1
...
✅ fanquake closed an issue: "SegFault in QSortFilterProxyModelPrivate::build_source_to_proxy_mapping"
(https://github.com/bitcoin/bitcoin/issues/32957)
(https://github.com/bitcoin/bitcoin/issues/32957)
💬 fanquake commented on issue "SegFault in QSortFilterProxyModelPrivate::build_source_to_proxy_mapping":
(https://github.com/bitcoin/bitcoin/issues/32957#issuecomment-3101967885)
Moved to https://github.com/bitcoin-core/gui/issues/880.
(https://github.com/bitcoin/bitcoin/issues/32957#issuecomment-3101967885)
Moved to https://github.com/bitcoin-core/gui/issues/880.
💬 fanquake commented on issue "cmake: Replace f`ile(GLOB ...)` command with an explicit list of `*.ts` files":
(https://github.com/bitcoin/bitcoin/issues/32653#issuecomment-3101971822)
PR open in the maintainer tools repo: https://github.com/bitcoin-core/bitcoin-maintainer-tools/pull/185.
(https://github.com/bitcoin/bitcoin/issues/32653#issuecomment-3101971822)
PR open in the maintainer tools repo: https://github.com/bitcoin-core/bitcoin-maintainer-tools/pull/185.
💬 Sjors commented on pull request "doc: Add release notes for 32521 (MAX_TX_LEGACY_SIGOPS)":
(https://github.com/bitcoin/bitcoin/pull/33037#issuecomment-3101974508)
Maybe mention that these rules match the proposed consensus changes in BIP54.
(https://github.com/bitcoin/bitcoin/pull/33037#issuecomment-3101974508)
Maybe mention that these rules match the proposed consensus changes in BIP54.
💬 maflcko commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#discussion_r2222082548)
Could also mention the RPC field removals?
Also, if you want to fix typos, the LLM found a few:
tranaction -> transaction [misspelling in comment “memory usage of the tranaction”, and "latency score for all tranactions"]
comphehensive -> comprehensive [misspelling in comment “This is a comphehensive simulation…”]
Remove trailing “)” in “We must now be within limits, otherwise LimitOrphans should have continued further).” [unmatched parenthesis]
But up to you.
(https://github.com/bitcoin/bitcoin/pull/32941#discussion_r2222082548)
Could also mention the RPC field removals?
Also, if you want to fix typos, the LLM found a few:
tranaction -> transaction [misspelling in comment “memory usage of the tranaction”, and "latency score for all tranactions"]
comphehensive -> comprehensive [misspelling in comment “This is a comphehensive simulation…”]
Remove trailing “)” in “We must now be within limits, otherwise LimitOrphans should have continued further).” [unmatched parenthesis]
But up to you.
💬 b-l-u-e commented on pull request "net: Fix Discover() not running when using -bind=0.0.0.0:port":
(https://github.com/bitcoin/bitcoin/pull/32757#issuecomment-3102261336)
Thanks for the thorough review and the helpful suggestions, @vasild
You are right. My initial assessment that the test framework was automatically adding an onion bind was incorrect; it is indeed **`bitcoind` itself** that handles the onion bind based on configuration.
Regarding your question about `nodes[1]` and `nodes[2]` conflicting, my apologies for the confusion in my previous comment. The primary port conflict I was encountering was not between `nodes[1]` and `nodes[2]`, but rather,
...
(https://github.com/bitcoin/bitcoin/pull/32757#issuecomment-3102261336)
Thanks for the thorough review and the helpful suggestions, @vasild
You are right. My initial assessment that the test framework was automatically adding an onion bind was incorrect; it is indeed **`bitcoind` itself** that handles the onion bind based on configuration.
Regarding your question about `nodes[1]` and `nodes[2]` conflicting, my apologies for the confusion in my previous comment. The primary port conflict I was encountering was not between `nodes[1]` and `nodes[2]`, but rather,
...
💬 Sjors commented on pull request "doc: Add release notes for 32521 (MAX_TX_LEGACY_SIGOPS)":
(https://github.com/bitcoin/bitcoin/pull/33037#issuecomment-3102303178)
ACK faa2f3b1afe706c6d44fa1949dd97c9e6df5c9d1
(https://github.com/bitcoin/bitcoin/pull/33037#issuecomment-3102303178)
ACK faa2f3b1afe706c6d44fa1949dd97c9e6df5c9d1
💬 Eunovo commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#issuecomment-3102421425)
> So in that case I think the bevavior should be:
>
> * if coins are sent to a bech32m address, use a silent payment change address (don't use the regular bech32m descriptor)
> * otherwise, use a matching output type
>
> * when there's no descriptor for it, fall back to silent payment change address
Done
(https://github.com/bitcoin/bitcoin/pull/32966#issuecomment-3102421425)
> So in that case I think the bevavior should be:
>
> * if coins are sent to a bech32m address, use a silent payment change address (don't use the regular bech32m descriptor)
> * otherwise, use a matching output type
>
> * when there's no descriptor for it, fall back to silent payment change address
Done
💬 yancyribbens commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2222306224)
> You appear to be talking about running a test with two different UTXO sets generated from the same set of effective values at different feerates. The latter is done already in the test I linked above. For the former the invariant you describe does not hold.
Yes, that is what I'm talking about said a different way. Yes, the test you linked above tests for that but only for the same static values.
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2222306224)
> You appear to be talking about running a test with two different UTXO sets generated from the same set of effective values at different feerates. The latter is done already in the test I linked above. For the former the invariant you describe does not hold.
Yes, that is what I'm talking about said a different way. Yes, the test you linked above tests for that but only for the same static values.
💬 kevkevinpal commented on pull request "test: functional test for incomplete PSBT with additional signatures required":
(https://github.com/bitcoin/bitcoin/pull/33035#issuecomment-3102434281)
I tried running the test and generating a coverage report but did not see any additional coverage, here is how I did it
---
[doc to build coverage report](https://github.com/bitcoin/bitcoin/blob/7129c9ea8e950f50bdc56d88c57617c66c90bb8a/doc/developer-notes.md#using-llvmclang-toolchain)
Below is the direct commands to generate the report for this test `rpc_psbt.py`
```
cmake -B build -DCMAKE_C_COMPILER="clang" \
-DCMAKE_CXX_COMPILER="clang++" \
-DAPPEND_CFLAGS="-fprofile-instr-g
...
(https://github.com/bitcoin/bitcoin/pull/33035#issuecomment-3102434281)
I tried running the test and generating a coverage report but did not see any additional coverage, here is how I did it
---
[doc to build coverage report](https://github.com/bitcoin/bitcoin/blob/7129c9ea8e950f50bdc56d88c57617c66c90bb8a/doc/developer-notes.md#using-llvmclang-toolchain)
Below is the direct commands to generate the report for this test `rpc_psbt.py`
```
cmake -B build -DCMAKE_C_COMPILER="clang" \
-DCMAKE_CXX_COMPILER="clang++" \
-DAPPEND_CFLAGS="-fprofile-instr-g
...
💬 yancyribbens commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2222316261)
> The test you propose is also interesting, and someone might want to take a stab at adding it in another pull request.
That would be great.
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2222316261)
> The test you propose is also interesting, and someone might want to take a stab at adding it in another pull request.
That would be great.
💬 yancyribbens commented on pull request "FUZZ: Test that BnB finds best solution":
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2222321444)
> UTXOs are ordered in descending effective value by BnB, so I don’t think the UTXO with largest value being likely to be generated first is a problem.
I was thinking of testing this more as you would a black box.
(https://github.com/bitcoin/bitcoin/pull/32894#discussion_r2222321444)
> UTXOs are ordered in descending effective value by BnB, so I don’t think the UTXO with largest value being likely to be generated first is a problem.
I was thinking of testing this more as you would a black box.