💬 vasild commented on pull request "net: Fix Discover() not running when using -bind=0.0.0.0:port":
(https://github.com/bitcoin/bitcoin/pull/32757#discussion_r2221491082)
The testing framework is designed so that it can run parallel executions on the same machine. For example, running two separate tests independently at the same time. To avoid port collisions between such independent executions is has `p2p_port(n)` and `tor_port(n)` which allocate ports in a range which (hopefully) will not overlap with the range from another test that executes concurrently (e.g. in another terminal). This `+100` seems a odds with this, maybe it could cause collisions.
I would
...
(https://github.com/bitcoin/bitcoin/pull/32757#discussion_r2221491082)
The testing framework is designed so that it can run parallel executions on the same machine. For example, running two separate tests independently at the same time. To avoid port collisions between such independent executions is has `p2p_port(n)` and `tor_port(n)` which allocate ports in a range which (hopefully) will not overlap with the range from another test that executes concurrently (e.g. in another terminal). This `+100` seems a odds with this, maybe it could cause collisions.
I would
...
💬 Eunovo 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_r2221513192)
To add this assertion, we will have to remove those checks, and I haven't seen the advantage in doing that.
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2221513192)
To add this assertion, we will have to remove those checks, and I haven't seen the advantage in doing that.
💬 Eunovo commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2221561291)
Yes, it does. The ImportingNow object is destroyed at the end of the function.
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2221561291)
Yes, it does. The ImportingNow object is destroyed at the end of the function.
💬 maflcko commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2221607401)
nit: you can use `ALWAYS_FALSE` for this, so that the workaround docs for the C++23 defect report are in one place only.
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2221607401)
nit: you can use `ALWAYS_FALSE` for this, so that the workaround docs for the C++23 defect report are in one place only.
💬 maflcko commented on pull request "tests: speed up coins_tests by parallelizing":
(https://github.com/bitcoin/bitcoin/pull/32945#issuecomment-3101567218)
lgtm ACK 06ab3a394ade1e0d4fdb1fef636ff0d6bad71948
(https://github.com/bitcoin/bitcoin/pull/32945#issuecomment-3101567218)
lgtm ACK 06ab3a394ade1e0d4fdb1fef636ff0d6bad71948
💬 maflcko commented on pull request "test: functional test for incomplete PSBT with additional signatures required":
(https://github.com/bitcoin/bitcoin/pull/33035#issuecomment-3101578061)
https://corecheck.dev/bitcoin/bitcoin/pulls/33035 does not show any new coverage. What is the new coverage and how to observe that it is actually new?
(https://github.com/bitcoin/bitcoin/pull/33035#issuecomment-3101578061)
https://corecheck.dev/bitcoin/bitcoin/pulls/33035 does not show any new coverage. What is the new coverage and how to observe that it is actually new?
💬 Eunovo commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2221693565)
Done.
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2221693565)
Done.
💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3101643309)
Perhaps there should be a (continuously updated) BIP of transaction standardness rules? It can track which clients and versions support them. And list which ones are configurable in clients. And finally which clients have different defaults.
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3101643309)
Perhaps there should be a (continuously updated) BIP of transaction standardness rules? It can track which clients and versions support them. And list which ones are configurable in clients. And finally which clients have different defaults.
💬 Sjors commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#issuecomment-3101665965)
@Eunovo that's an interesting trade-off, and we should probably be consistent with how the rest of the wallet behaves. Otherwise we'd leave a fingerprint of which coins were sent using silent payments.
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
(https://github.com/bitcoin/bitcoin/pull/32966#issuecomment-3101665965)
@Eunovo that's an interesting trade-off, and we should probably be consistent with how the rest of the wallet behaves. Otherwise we'd leave a fingerprint of which coins were sent using silent payments.
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
📝 fanquake opened a pull request: "Update secp256k1 subtree to latest master"
(https://github.com/bitcoin/bitcoin/pull/33036)
Updates the subtree to https://github.com/bitcoin-core/secp256k1/commit/b9313c6e1a6082a66b4c75777e18ca4b176fcf9d. See https://github.com/bitcoin-core/secp256k1/blob/master/CHANGELOG.md#070---2025-07-21 for the most relevant changes.
(https://github.com/bitcoin/bitcoin/pull/33036)
Updates the subtree to https://github.com/bitcoin-core/secp256k1/commit/b9313c6e1a6082a66b4c75777e18ca4b176fcf9d. See https://github.com/bitcoin-core/secp256k1/blob/master/CHANGELOG.md#070---2025-07-21 for the most relevant changes.
💬 Sjors commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-3101714725)
This needed a release note :-)
Maybe we can still mention it in the next v29 update?
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-3101714725)
This needed a release note :-)
Maybe we can still mention it in the next v29 update?
💬 fanquake commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-3101723352)
https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes/release-notes-29.0.md#build-system
> By default, the built executables and libraries are located in the bin/ and lib/ subdirectories of the build directory.
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-3101723352)
https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes/release-notes-29.0.md#build-system
> By default, the built executables and libraries are located in the bin/ and lib/ subdirectories of the build directory.
📝 maflcko opened a pull request: "doc: Add release notes for 32521 (MAX_TX_LEGACY_SIGOPS)"
(https://github.com/bitcoin/bitcoin/pull/33037)
ref https://github.com/bitcoin/bitcoin/pull/32521
(https://github.com/bitcoin/bitcoin/pull/33037)
ref https://github.com/bitcoin/bitcoin/pull/32521
💬 Eunovo commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2221785394)
`ImportBlocks` first tries to load blocks from existing block files during the reindex process. After the reindex process, it tries to load any external blocks provided by the user. We only want to force the use of assumevalid(if set) during the reindex process, not when loading external blocks.
We call `activate_all_chainstate` the first time, during the reindex process. This allows us to skip script checks using modified assumevalid rules for reindex.
We call `activate_all_chainstate` the
...
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2221785394)
`ImportBlocks` first tries to load blocks from existing block files during the reindex process. After the reindex process, it tries to load any external blocks provided by the user. We only want to force the use of assumevalid(if set) during the reindex process, not when loading external blocks.
We call `activate_all_chainstate` the first time, during the reindex process. This allows us to skip script checks using modified assumevalid rules for reindex.
We call `activate_all_chainstate` the
...
💬 maflcko commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3101732454)
Created https://github.com/bitcoin/bitcoin/pull/33037 and removed the 'needs release notes' label.
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3101732454)
Created https://github.com/bitcoin/bitcoin/pull/33037 and removed the 'needs release notes' label.
💬 maflcko commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3101773916)
> It can track which clients and versions support them. And list which ones are configurable in clients. And finally which clients have different defaults.
You can use `$ ./bld-cmake/bin/bitcoin-qt -? | grep 'Node relay options:'` to get the list of configurable standardness rules and their default values for all versions you are interested in. There is also https://github.com/bitcoin/bitcoin/tree/master/doc/policy, but it isn't complete. I am not sure if it makes sense to try to list each th
...
(https://github.com/bitcoin/bitcoin/pull/32521#issuecomment-3101773916)
> It can track which clients and versions support them. And list which ones are configurable in clients. And finally which clients have different defaults.
You can use `$ ./bld-cmake/bin/bitcoin-qt -? | grep 'Node relay options:'` to get the list of configurable standardness rules and their default values for all versions you are interested in. There is also https://github.com/bitcoin/bitcoin/tree/master/doc/policy, but it isn't complete. I am not sure if it makes sense to try to list each th
...
🤔 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?