💬 furszy commented on pull request "wallet: reduce unconditional logging during load":
(https://github.com/bitcoin/bitcoin/pull/33299#issuecomment-3352513225)
> At the moment, we use Berkeley DB when we want to migrate a Legacy wallet, the above statement maybe should consider this case.
You can't initialize a BDB wallet during startup. The BDB engine is not part of the binary anymore. We just have a BDB reader that parses the db file on-demand during migration.
(https://github.com/bitcoin/bitcoin/pull/33299#issuecomment-3352513225)
> At the moment, we use Berkeley DB when we want to migrate a Legacy wallet, the above statement maybe should consider this case.
You can't initialize a BDB wallet during startup. The BDB engine is not part of the binary anymore. We just have a BDB reader that parses the db file on-demand during migration.
🤔 hebasto reviewed a pull request: "CMake: Add dynamic test discovery"
(https://github.com/bitcoin/bitcoin/pull/33483#pullrequestreview-3285383953)
> Instead of parsing the test names from the source code at configure time, query the list of tests from the test executables at testing time.
It might also be helpful to explain *why* this approach is an improvement.
The current implementation does not handle skipped tests correctly:
- on the master branch:
```
$ ctest --test-dir build -j 20
<snip>
73/148 Test #88: script_assets_tests ..................***Skipped 0.13 sec
<snip>
100% tests passed, 0 tests failed out of 148
...
(https://github.com/bitcoin/bitcoin/pull/33483#pullrequestreview-3285383953)
> Instead of parsing the test names from the source code at configure time, query the list of tests from the test executables at testing time.
It might also be helpful to explain *why* this approach is an improvement.
The current implementation does not handle skipped tests correctly:
- on the master branch:
```
$ ctest --test-dir build -j 20
<snip>
73/148 Test #88: script_assets_tests ..................***Skipped 0.13 sec
<snip>
100% tests passed, 0 tests failed out of 148
...
💬 hebasto commented on pull request "CMake: Add dynamic test discovery":
(https://github.com/bitcoin/bitcoin/pull/33483#discussion_r2391884367)
The property name started with `CTEST_` seems to be reserved, no?
(https://github.com/bitcoin/bitcoin/pull/33483#discussion_r2391884367)
The property name started with `CTEST_` seems to be reserved, no?
💬 mzumsande commented on pull request "p2p: Use network-dependent timers for inbound inv scheduling":
(https://github.com/bitcoin/bitcoin/pull/33464#discussion_r2391996003)
Good question - while the map initially empty, we insert entries with
`auto& timer{m_next_inv_to_inbounds_per_network_key[network_key]};`
Even though default-initialisation doesn't happen in general for `std::atomic`, this should still be correct because `std::map:operator[]` uses _value initialisation_ (to 0).
That said, I think it's better to use `try_emplace` to make the intent clearer, so I will change it to that.
(https://github.com/bitcoin/bitcoin/pull/33464#discussion_r2391996003)
Good question - while the map initially empty, we insert entries with
`auto& timer{m_next_inv_to_inbounds_per_network_key[network_key]};`
Even though default-initialisation doesn't happen in general for `std::atomic`, this should still be correct because `std::map:operator[]` uses _value initialisation_ (to 0).
That said, I think it's better to use `try_emplace` to make the intent clearer, so I will change it to that.
💬 mzumsande commented on pull request "p2p: Use network-dependent timers for inbound inv scheduling":
(https://github.com/bitcoin/bitcoin/pull/33464#issuecomment-3352747761)
[beb75e4](https://github.com/bitcoin/bitcoin/commit/beb75e48ae1d5771932f427a490c7e1b6c1720d3) to [0f7d4ee](https://github.com/bitcoin/bitcoin/commit/0f7d4ee4e8281ed141a6ebb7e0edee7b864e4dcf):
- used `try_emplace` instead of `[]` for map insertion
- Removed the atomic and guarded `m_next_inv_to_inbounds_per_network_key` by `g_msgproc_mutex`. `NextInvToInbounds` is already guarded by `g_msgproc_mutex` anyway, so I'm not sure what the benefit of the atomic was (there was even a comment, removed n
...
(https://github.com/bitcoin/bitcoin/pull/33464#issuecomment-3352747761)
[beb75e4](https://github.com/bitcoin/bitcoin/commit/beb75e48ae1d5771932f427a490c7e1b6c1720d3) to [0f7d4ee](https://github.com/bitcoin/bitcoin/commit/0f7d4ee4e8281ed141a6ebb7e0edee7b864e4dcf):
- used `try_emplace` instead of `[]` for map insertion
- Removed the atomic and guarded `m_next_inv_to_inbounds_per_network_key` by `g_msgproc_mutex`. `NextInvToInbounds` is already guarded by `g_msgproc_mutex` anyway, so I'm not sure what the benefit of the atomic was (there was even a comment, removed n
...
💬 optout21 commented on pull request "net: support overriding the proxy selection in ConnectNode()":
(https://github.com/bitcoin/bitcoin/pull/33454#issuecomment-3352795379)
ACK 0c718da693ad4727009e656938a1208ec52866af
Fairly reduced scope; motivated by #29415 .
(https://github.com/bitcoin/bitcoin/pull/33454#issuecomment-3352795379)
ACK 0c718da693ad4727009e656938a1208ec52866af
Fairly reduced scope; motivated by #29415 .
⚠️ ismaelsadeeq opened an issue: "Mempool Expiry eviction might remove txs that could be mined in the next block"
(https://github.com/bitcoin/bitcoin/issues/33510)
`LimitMempoolSize` always try to remove transactions from the mempool whose age has passed the mempool expiry time limit, which is 336 hours (i.e., two weeks) by default.
There might be a case where you evict something that might be mined next. For example, the mempool was cleared and some low feerate transaction that has been in the mempool and has not confirmed in a long time due to having a low feerate is now at the top of the mempool.
However, when we want to limit the mempool size, we will
...
(https://github.com/bitcoin/bitcoin/issues/33510)
`LimitMempoolSize` always try to remove transactions from the mempool whose age has passed the mempool expiry time limit, which is 336 hours (i.e., two weeks) by default.
There might be a case where you evict something that might be mined next. For example, the mempool was cleared and some low feerate transaction that has been in the mempool and has not confirmed in a long time due to having a low feerate is now at the top of the mempool.
However, when we want to limit the mempool size, we will
...
💬 theStack commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2392113431)
potential refactoring idea: the derivation path lookup prior to calling `SignMuSig2` currently happens for both key- and script-path spending. Could it be moved inside of `SignMuSig2` in order to deduplicate (and also reduce the interface by one parameter, as the `KeyOriginInfo` object would be created inside)? Tried this here, and the tests still pass: https://github.com/theStack/bitcoin/commit/fc20504660aed9674ae27482fad885b5b9fe399f
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2392113431)
potential refactoring idea: the derivation path lookup prior to calling `SignMuSig2` currently happens for both key- and script-path spending. Could it be moved inside of `SignMuSig2` in order to deduplicate (and also reduce the interface by one parameter, as the `KeyOriginInfo` object would be created inside)? Tried this here, and the tests still pass: https://github.com/theStack/bitcoin/commit/fc20504660aed9674ae27482fad885b5b9fe399f
💬 ismaelsadeeq commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#issuecomment-3352926498)
Approach ACK
I ran this branch for a while on mainnet and have not noticed any issue, importing of mempool saved before cluster succeeded in multiple runs.
> Would it be a good idea to split this PR into a main PR with Cluster Implementation + Cleanups + Docs and Tests, then a followups PR with Optimizations and more cleanups? Goal is to get everything in v31, but this might help with reducing the size of the main PR and make it easier to punt cleanup-related comments to later.
I've sp
...
(https://github.com/bitcoin/bitcoin/pull/28676#issuecomment-3352926498)
Approach ACK
I ran this branch for a while on mainnet and have not noticed any issue, importing of mempool saved before cluster succeeded in multiple runs.
> Would it be a good idea to split this PR into a main PR with Cluster Implementation + Cleanups + Docs and Tests, then a followups PR with Optimizations and more cleanups? Goal is to get everything in v31, but this might help with reducing the size of the main PR and make it easier to punt cleanup-related comments to later.
I've sp
...
💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392172151)
The `-disablewallet` error was caused by `self.uses_wallet = True`, not MiniWallet. The test framework starts nodes with `-disablewallet` when `uses_wallet = True` in no-wallet CI environments. MiniWallet doesn't need wallet RPC (it uses `sendrawtransaction`), so changing to `uses_wallet = None` allows the test to run in no-wallet environments while still using MiniWallet for transaction creation.
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392172151)
The `-disablewallet` error was caused by `self.uses_wallet = True`, not MiniWallet. The test framework starts nodes with `-disablewallet` when `uses_wallet = True` in no-wallet CI environments. MiniWallet doesn't need wallet RPC (it uses `sendrawtransaction`), so changing to `uses_wallet = None` allows the test to run in no-wallet environments while still using MiniWallet for transaction creation.
💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392173987)
Recent changes uses a different approach, so there is no need for this anymore
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392173987)
Recent changes uses a different approach, so there is no need for this anymore
💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3352950893)
> > they weren't legacy wallet specific but rather configuration problems.
>
> You'll have to adjust the pull description. Also, CI fails.
>
> To ensure this is tested once, you can add `--gen-test-data` to one CI task config via TEST_RUNNER_EXTRA.
Updated the PR title and description
The recent simplification changes make adding a CI task config unnecessary. The test now always generates data with MiniWallet by default, so the data generation path is automatically tested in all CI e
...
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3352950893)
> > they weren't legacy wallet specific but rather configuration problems.
>
> You'll have to adjust the pull description. Also, CI fails.
>
> To ensure this is tested once, you can add `--gen-test-data` to one CI task config via TEST_RUNNER_EXTRA.
Updated the PR title and description
The recent simplification changes make adding a CI task config unnecessary. The test now always generates data with MiniWallet by default, so the data generation path is automatically tested in all CI e
...
💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3352960402)
> I think this test could be simplified to always generate data with the MiniWallet and test with generated data, instead of dumping it to a file and loading it later. I don't see the advantage of "generate" and "load" paths, and it's creating unnecessary complexity.
> I think this test could be simplified to always generate data with the MiniWallet and test with generated data, instead of dumping it to a file and loading it later. I don't see the advantage of "generate" and "load" paths,
...
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3352960402)
> I think this test could be simplified to always generate data with the MiniWallet and test with generated data, instead of dumping it to a file and loading it later. I don't see the advantage of "generate" and "load" paths, and it's creating unnecessary complexity.
> I think this test could be simplified to always generate data with the MiniWallet and test with generated data, instead of dumping it to a file and loading it later. I don't see the advantage of "generate" and "load" paths,
...
💬 m3dwards commented on pull request "ci: fix buildx gha cache authentication on forks":
(https://github.com/bitcoin/bitcoin/pull/33508#issuecomment-3352975826)
ACK bc706955d740f8a59bec78e44d33e80d1cca373b
> Fork run ongoing at: https://github.com/willcl-ark/bitcoin/actions/runs/18126864863
As it wasn't master, I've also pushed to my master to also check writing to the cache: https://github.com/m3dwards/bitcoin/actions/runs/18136688725/job/51617223612
And I can see all is well:
```shell
#12 exporting to GitHub Actions Cache
#12 preparing build cache for export
#12 writing layer sha256:a671ea418149154b4e0f1d5670be077da1a48f411402479d8fc137
...
(https://github.com/bitcoin/bitcoin/pull/33508#issuecomment-3352975826)
ACK bc706955d740f8a59bec78e44d33e80d1cca373b
> Fork run ongoing at: https://github.com/willcl-ark/bitcoin/actions/runs/18126864863
As it wasn't master, I've also pushed to my master to also check writing to the cache: https://github.com/m3dwards/bitcoin/actions/runs/18136688725/job/51617223612
And I can see all is well:
```shell
#12 exporting to GitHub Actions Cache
#12 preparing build cache for export
#12 writing layer sha256:a671ea418149154b4e0f1d5670be077da1a48f411402479d8fc137
...
💬 jonatack commented on pull request "p2p: Use network-dependent timers for inbound inv scheduling":
(https://github.com/bitcoin/bitcoin/pull/33464#issuecomment-3352985083)
Concept ACK. Good to see proposals like this and https://github.com/bitcoin/bitcoin/pull/33498 to address fingerprinting.
(https://github.com/bitcoin/bitcoin/pull/33464#issuecomment-3352985083)
Concept ACK. Good to see proposals like this and https://github.com/bitcoin/bitcoin/pull/33498 to address fingerprinting.
💬 jonatack commented on pull request "p2p: Mitigate GETADDR fingerprinting by setting address timestamps to a fixed value":
(https://github.com/bitcoin/bitcoin/pull/33498#issuecomment-3352992312)
Concept ACK on working on this. Good to see proposals like this and #33464.
(https://github.com/bitcoin/bitcoin/pull/33498#issuecomment-3352992312)
Concept ACK on working on this. Good to see proposals like this and #33464.
💬 ajtowns commented on issue "[29.x] guix build failure on ppc with xcb":
(https://github.com/bitcoin/bitcoin/issues/33488#issuecomment-3353006087)
No? I just untarred the Xcode SDK in a clean environment, set HOSTS and ran guix-build. The depends/sources files all seem to match the hashes from depends/packages as far as I can see for what that's worth.
(https://github.com/bitcoin/bitcoin/issues/33488#issuecomment-3353006087)
No? I just untarred the Xcode SDK in a clean environment, set HOSTS and ran guix-build. The depends/sources files all seem to match the hashes from depends/packages as far as I can see for what that's worth.
💬 maflcko commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3353039818)
> **Changes:**
>
> * Changed `self.uses_wallet = True` to `self.uses_wallet = None` to allow no-wallet environments
>
> * Added `self.skip_if_no_wallet()` when generating test data to ensure wallet is available when needed
>
> * Added `--gen-test-data` to TSan CI via `TEST_RUNNER_EXTRA` to ensure data generation is tested
>
>
> **How it works:**
>
> * In no-wallet environments: Test loads pre-generated data (no wallet required)
>
> * With `--gen-test-data`
...
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3353039818)
> **Changes:**
>
> * Changed `self.uses_wallet = True` to `self.uses_wallet = None` to allow no-wallet environments
>
> * Added `self.skip_if_no_wallet()` when generating test data to ensure wallet is available when needed
>
> * Added `--gen-test-data` to TSan CI via `TEST_RUNNER_EXTRA` to ensure data generation is tested
>
>
> **How it works:**
>
> * In no-wallet environments: Test loads pre-generated data (no wallet required)
>
> * With `--gen-test-data`
...
💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3353093426)
> Generally, instead of just repeating the changes made, it could make sense to motivate them and explain why each change makes sense and is correct.
Updated PR description to explain motivation rather than just listing changes.
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3353093426)
> Generally, instead of just repeating the changes made, it could make sense to motivate them and explain why each change makes sense and is correct.
Updated PR description to explain motivation rather than just listing changes.
👍 stickies-v approved a pull request: "[28.x] backports + 28.3rc1"
(https://github.com/bitcoin/bitcoin/pull/33476#pullrequestreview-3285604677)
ACK dcac36271f9c6d47a863ab638a91382be6f7a50f , although I'm a bit confused about one change (see comment) that doesn't seem like a backport (but not necessarily bad by itself).
I verified all backports are clean, except for a bunch of test-only merge conflicts in 33106:
- cf875f15596052d42abccecb81a96d8e343fd2cd backported from 72dc18467dbfc16cdbda2dd109b087243b397799: addressed merge conflict from c16ae717689295338025dde74c0a7a51965c383f and re-added `-datacarriersize` arg because of 9f36
...
(https://github.com/bitcoin/bitcoin/pull/33476#pullrequestreview-3285604677)
ACK dcac36271f9c6d47a863ab638a91382be6f7a50f , although I'm a bit confused about one change (see comment) that doesn't seem like a backport (but not necessarily bad by itself).
I verified all backports are clean, except for a bunch of test-only merge conflicts in 33106:
- cf875f15596052d42abccecb81a96d8e343fd2cd backported from 72dc18467dbfc16cdbda2dd109b087243b397799: addressed merge conflict from c16ae717689295338025dde74c0a7a51965c383f and re-added `-datacarriersize` arg because of 9f36
...