💬 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_r2221456230)
Nice idea. Added you as Co-author in the relevant commit.
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2221456230)
Nice idea. Added you as Co-author in the relevant commit.
💬 josibake commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2221476916)
> The silent payments wallet needs to scan entire blocks since it doesn't know ahead of time which scriptPubKeys to use in a GCS filter
This is correct. I was imagining a world where we can pre-compute the scriptPubKeys, but realised we have to iterate the blocks anyways to get the tweaks so its likely faster to just check directly. In the future, if the wallet has some sort of silent payments index, then I think fast rescans would be possible (and beneficial).
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2221476916)
> The silent payments wallet needs to scan entire blocks since it doesn't know ahead of time which scriptPubKeys to use in a GCS filter
This is correct. I was imagining a world where we can pre-compute the scriptPubKeys, but realised we have to iterate the blocks anyways to get the tweaks so its likely faster to just check directly. In the future, if the wallet has some sort of silent payments index, then I think fast rescans would be possible (and beneficial).
💬 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_r2221504575)
I forgot about the guard I added in https://github.com/bitcoin/bitcoin/pull/32471/commits/4202dca524aeef13e09380804c483d7c5402092d
You are right in the sense that it's not possible for a watch-only to even get to `ToPrivateString()` through the rpc; we could assert `has_priv_key` here. However, the descriptor unit tests often call `ToPrivateString()` on watch-only descriptors; assert `has_priv_key` causes the tests to fail.
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2221504575)
I forgot about the guard I added in https://github.com/bitcoin/bitcoin/pull/32471/commits/4202dca524aeef13e09380804c483d7c5402092d
You are right in the sense that it's not possible for a watch-only to even get to `ToPrivateString()` through the rpc; we could assert `has_priv_key` here. However, the descriptor unit tests often call `ToPrivateString()` on watch-only descriptors; assert `has_priv_key` causes the tests to fail.
🤔 vasild reviewed a pull request: "net: Fix Discover() not running when using -bind=0.0.0.0:port"
(https://github.com/bitcoin/bitcoin/pull/32757#pullrequestreview-3041523676)
Almost ACK a9b7542b6ea1af770d93cbbf5bb55dae6841334f
> I encountered port conflicts because the test framework automatically adds an onion bind to node 2,
Hmm, I think it is not the test framework but `bitcoind` itself that adds the onion bind. To figure out how `bitcoind` is started, e.g. for `nodes[2]` I look at `/tmp/bitcoin_func_test_.../node2/regtest/debug.log` for the lines like:
```
...
Config file arg: [regtest] shrinkdebugfile="0"
Config file arg: [regtest] unsafesqlitesync="1
...
(https://github.com/bitcoin/bitcoin/pull/32757#pullrequestreview-3041523676)
Almost ACK a9b7542b6ea1af770d93cbbf5bb55dae6841334f
> I encountered port conflicts because the test framework automatically adds an onion bind to node 2,
Hmm, I think it is not the test framework but `bitcoind` itself that adds the onion bind. To figure out how `bitcoind` is started, e.g. for `nodes[2]` I look at `/tmp/bitcoin_func_test_.../node2/regtest/debug.log` for the lines like:
```
...
Config file arg: [regtest] shrinkdebugfile="0"
Config file arg: [regtest] unsafesqlitesync="1
...
💬 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
...