💬 maflcko commented on pull request "wallet: Remove `upgradewallet` RPC":
(https://github.com/bitcoin/bitcoin/pull/32944#issuecomment-3079400287)
> I think the new migrated wallet has the latest version of features (`walletInstance->SetMinVersion(FEATURE_LATEST)`)
I don't think this is true, at least locally:
```
> migratewallet ""
< {
"wallet_name": "",
"backup_path": "/tmp/regtest/default_wallet_1752683462.legacy.bak"
}
> getwalletinfo
< {
"walletname": "",
"walletversion": 130000,
"format": "sqlite",
"txcount": 0,
"keypoolsize": 4000,
"paytxfee": 0.00000000,
"private_keys_enabled": true,
"avoi
...
(https://github.com/bitcoin/bitcoin/pull/32944#issuecomment-3079400287)
> I think the new migrated wallet has the latest version of features (`walletInstance->SetMinVersion(FEATURE_LATEST)`)
I don't think this is true, at least locally:
```
> migratewallet ""
< {
"wallet_name": "",
"backup_path": "/tmp/regtest/default_wallet_1752683462.legacy.bak"
}
> getwalletinfo
< {
"walletname": "",
"walletversion": 130000,
"format": "sqlite",
"txcount": 0,
"keypoolsize": 4000,
"paytxfee": 0.00000000,
"private_keys_enabled": true,
"avoi
...
💬 pablomartin4btc commented on pull request "wallet: Remove `upgradewallet` RPC":
(https://github.com/bitcoin/bitcoin/pull/32944#issuecomment-3079415883)
Ok, thanks for testing it. In that case, that needs to be fixed, keeping the field or not, the returned wallet version for a descriptor should be the latest, unless I'm missing something...
(https://github.com/bitcoin/bitcoin/pull/32944#issuecomment-3079415883)
Ok, thanks for testing it. In that case, that needs to be fixed, keeping the field or not, the returned wallet version for a descriptor should be the latest, unless I'm missing something...
💬 josibake commented on issue "ci: improve "test each commit" job to handle more complex scenarios":
(https://github.com/bitcoin/bitcoin/issues/32991#issuecomment-3079421580)
> I can't reproduce this locally
Let me know if I can help with reproducing this. I haven't tried to reproduce the CI failure locally, but I can recreate a similar failure by first creating a branch where I pull in the latest secp subtree, and then try to rebase my branch on top of that. This will fail with a merge conflict message, unless I use `git rebase --rebase-merges --strategy subtree`.
I'll also try digging into the CI task a bit more, later this week.
(https://github.com/bitcoin/bitcoin/issues/32991#issuecomment-3079421580)
> I can't reproduce this locally
Let me know if I can help with reproducing this. I haven't tried to reproduce the CI failure locally, but I can recreate a similar failure by first creating a branch where I pull in the latest secp subtree, and then try to rebase my branch on top of that. This will fail with a merge conflict message, unless I use `git rebase --rebase-merges --strategy subtree`.
I'll also try digging into the CI task a bit more, later this week.
💬 maflcko commented on issue "ci: improve "test each commit" job to handle more complex scenarios":
(https://github.com/bitcoin/bitcoin/issues/32991#issuecomment-3079438641)
I think when you rebase on top of a subtree merge, you'll want to do `git rebase --interactive c42c7351a6316805fdfb20163762c1ab8293dd0d` and then delete the old subtree commits:
```
# delete ! 497f1536ef # Squashed 'src/secp256k1/' changes from 4187a46649..c0db6509bd
# delete ! bb0dc6d75c # Squashed 'src/secp256k1/' changes from c0db6509bd..6264c3d093
pick da3494063f # crypto: add read-only method to KeyPair
pick 687eb1c44f # Add "sp" HRP
pick ebc062ba2a # Add V0SilentPaymentDestination address
...
(https://github.com/bitcoin/bitcoin/issues/32991#issuecomment-3079438641)
I think when you rebase on top of a subtree merge, you'll want to do `git rebase --interactive c42c7351a6316805fdfb20163762c1ab8293dd0d` and then delete the old subtree commits:
```
# delete ! 497f1536ef # Squashed 'src/secp256k1/' changes from 4187a46649..c0db6509bd
# delete ! bb0dc6d75c # Squashed 'src/secp256k1/' changes from c0db6509bd..6264c3d093
pick da3494063f # crypto: add read-only method to KeyPair
pick 687eb1c44f # Add "sp" HRP
pick ebc062ba2a # Add V0SilentPaymentDestination address
...
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2210979850)
`refactor: prepare DBWrapper for obfuscation key change` adds key size validation on read, clarifies that we have to turn off obfuscation before we read the vector, and inlines `CreateObfuscation` (+ renames to clarify things + narrowing of scopes).
Before, we could read directly into the `CDBWrapper` field, now we have to read into a temp vector and init the `Obfuscation` field from that.
If you have a better idea of how to do this, please let me know.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2210979850)
`refactor: prepare DBWrapper for obfuscation key change` adds key size validation on read, clarifies that we have to turn off obfuscation before we read the vector, and inlines `CreateObfuscation` (+ renames to clarify things + narrowing of scopes).
Before, we could read directly into the `CDBWrapper` field, now we have to read into a temp vector and init the `Obfuscation` field from that.
If you have a better idea of how to do this, please let me know.
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2210980097)
Yes, it's redundant, but I wanted to make absolutely sure the internals are also correct.
If other reviewers also comfortable deleting this, I don't mind.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2210980097)
Yes, it's redundant, but I wanted to make absolutely sure the internals are also correct.
If other reviewers also comfortable deleting this, I don't mind.
💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2211000449)
Ok, I see what you meant, the "wallet_name" parameter could be named differently from outside/ the RPC call. Is it possible to pass the request.param object itself? And get its name from it? I couldn't find any usage of it and also checked the UniValue lib... maybe I missed it, I'll double check but that would solve the potential issue that could happen as you describe as if a dev creates another RPC calling this function and not using "wallet_name" as an argument of the RPC call (some new RPC c
...
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2211000449)
Ok, I see what you meant, the "wallet_name" parameter could be named differently from outside/ the RPC call. Is it possible to pass the request.param object itself? And get its name from it? I couldn't find any usage of it and also checked the UniValue lib... maybe I missed it, I'll double check but that would solve the potential issue that could happen as you describe as if a dev creates another RPC calling this function and not using "wallet_name" as an argument of the RPC call (some new RPC c
...
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211021487)
Good observation: it's rather a safety, since `if (target.size() > KEY_SIZE) {` condition guarding it is just an optimization: everything works without it as well.
If we remove the `min`, the above condition isn't just an optimization anymore. I prefer keeping the redundancy, but if other reviewers think it's cleaner without, let me know.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211021487)
Good observation: it's rather a safety, since `if (target.size() > KEY_SIZE) {` condition guarding it is just an optimization: everything works without it as well.
If we remove the `min`, the above condition isn't just an optimization anymore. I prefer keeping the redundancy, but if other reviewers think it's cleaner without, let me know.
💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211030320)
I don't think there is any risk in reading directly into the field. The deser throws internally on key size validation, so it safe.
Handling with a vector separately just seems like extra handling/code?
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211030320)
I don't think there is any risk in reading directly into the field. The deser throws internally on key size validation, so it safe.
Handling with a vector separately just seems like extra handling/code?
💬 w0xlt commented on pull request "wallet: Remove `upgradewallet` RPC":
(https://github.com/bitcoin/bitcoin/pull/32944#issuecomment-3079524346)
If I understand correctly, the `walletversion` (`CWallet::nWalletVersion`) is not used by descriptor wallets (as can be checked in https://github.com/bitcoin/bitcoin/pull/32977), so it has no real effect on whether or not this field is updated after a migration.
Unless we intend to use this field in the future, I don't see the point in keeping it or this RPC.
(https://github.com/bitcoin/bitcoin/pull/32944#issuecomment-3079524346)
If I understand correctly, the `walletversion` (`CWallet::nWalletVersion`) is not used by descriptor wallets (as can be checked in https://github.com/bitcoin/bitcoin/pull/32977), so it has no real effect on whether or not this field is updated after a migration.
Unless we intend to use this field in the future, I don't see the point in keeping it or this RPC.
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211041788)
Seems simpler and I assumed copy elision kicks in, but I'll change it next time I touch the code, thanks.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211041788)
Seems simpler and I assumed copy elision kicks in, but I'll change it next time I touch the code, thanks.
💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2211042123)
Thanks! Makes sense, I'll add them.
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2211042123)
Thanks! Makes sense, I'll add them.
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211055488)
We're not exposing the internals of `Obfuscation`, we can't currently call `GetRandBytes(new_key)` on the internals, we have to first read the vector or create a random vector to be able to construct the object.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211055488)
We're not exposing the internals of `Obfuscation`, we can't currently call `GetRandBytes(new_key)` on the internals, we have to first read the vector or create a random vector to be able to construct the object.
💬 glozow commented on pull request "validation: docs and cleanups for MemPoolAccept coins views":
(https://github.com/bitcoin/bitcoin/pull/32973#discussion_r2211062513)
Yes. Should I reword?
(https://github.com/bitcoin/bitcoin/pull/32973#discussion_r2211062513)
Yes. Should I reword?
💬 glozow commented on pull request "validation: docs and cleanups for MemPoolAccept coins views":
(https://github.com/bitcoin/bitcoin/pull/32973#issuecomment-3079572074)
> op and commit message: `test_mid_package_evaluation` doesn't exist, it's named `test_mid_package_eviction`
Fixed
> The bull case for keeping `test_mid_package_replacement` is that it involves an RBF, not a mempool overflow, right? The comment in that test seems slightly erroneous, mentioning `replaced_tx` needs to be at the bottom of the mempool, when I think it just has to be low feerate enough to be RBF'd?
Thanks, yeah, I think it had an incorrect copy-paste from the other test. I t
...
(https://github.com/bitcoin/bitcoin/pull/32973#issuecomment-3079572074)
> op and commit message: `test_mid_package_evaluation` doesn't exist, it's named `test_mid_package_eviction`
Fixed
> The bull case for keeping `test_mid_package_replacement` is that it involves an RBF, not a mempool overflow, right? The comment in that test seems slightly erroneous, mentioning `replaced_tx` needs to be at the bottom of the mempool, when I think it just has to be low feerate enough to be RBF'd?
Thanks, yeah, I think it had an incorrect copy-paste from the other test. I t
...
💬 l0rinc commented on pull request "[IBD] log start of script validation past `assumevalid` block":
(https://github.com/bitcoin/bitcoin/pull/32975#issuecomment-3079579547)
Ran a reindex until 890k without `-assumevalid` to make sure it works for an actual IBD/reindex as well:
```python
2025-07-16T04:43:03Z UpdateTip: new best=00000000000000000001b658dd1120e82e66d2790811f89ede9742ada3ed6d77 height=886157 version=0x22024000 log2_work=95.475345 tx=1161875261 date='2025-03-03T15:52:21Z' progress=0.955815 cache=4682.3MiB(34814627txo)
2025-07-16T04:43:03Z Started signature validation at block 000000000000000000022614dc170eadb7b544824cbcdb0732bdeca42839aa37.
2025-07-
...
(https://github.com/bitcoin/bitcoin/pull/32975#issuecomment-3079579547)
Ran a reindex until 890k without `-assumevalid` to make sure it works for an actual IBD/reindex as well:
```python
2025-07-16T04:43:03Z UpdateTip: new best=00000000000000000001b658dd1120e82e66d2790811f89ede9742ada3ed6d77 height=886157 version=0x22024000 log2_work=95.475345 tx=1161875261 date='2025-03-03T15:52:21Z' progress=0.955815 cache=4682.3MiB(34814627txo)
2025-07-16T04:43:03Z Started signature validation at block 000000000000000000022614dc170eadb7b544824cbcdb0732bdeca42839aa37.
2025-07-
...
💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211075810)
`FastRandomContext` calls `GetRandBytes` internally, so I think it is fine to use (it is also used in the blockmanager).
At least for me the following compiles/links: (Obviously the code is wrong, I just wanted to check compilation)
```diff
diff --git a/src/dbwrapper.cpp b/src/dbwrapper.cpp
index 2b33e04468..79c909b552 100644
--- a/src/dbwrapper.cpp
+++ b/src/dbwrapper.cpp
@@ -252,13 +252,13 @@ CDBWrapper::CDBWrapper(const DBParams& params)
{
assert(!m_obfuscation); //
...
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211075810)
`FastRandomContext` calls `GetRandBytes` internally, so I think it is fine to use (it is also used in the blockmanager).
At least for me the following compiles/links: (Obviously the code is wrong, I just wanted to check compilation)
```diff
diff --git a/src/dbwrapper.cpp b/src/dbwrapper.cpp
index 2b33e04468..79c909b552 100644
--- a/src/dbwrapper.cpp
+++ b/src/dbwrapper.cpp
@@ -252,13 +252,13 @@ CDBWrapper::CDBWrapper(const DBParams& params)
{
assert(!m_obfuscation); //
...
💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211078238)
It only checks `m_rotations[0]` against a hardcoded memcpy, it doesn't check `ToKey` in the top commit, so I don't think the unit test adds any new checks. Otherwise, it would be good to know what exact bug this could possibly catch that the other test isn't catching.
In fact the unit test seems a bit confusing, because it constructs the obfuscation from an uint64_t, whereas production code only uses byte spans.
If you really want to directly test `Serialize` uses `m_rotations[0]` and `Un
...
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211078238)
It only checks `m_rotations[0]` against a hardcoded memcpy, it doesn't check `ToKey` in the top commit, so I don't think the unit test adds any new checks. Otherwise, it would be good to know what exact bug this could possibly catch that the other test isn't catching.
In fact the unit test seems a bit confusing, because it constructs the obfuscation from an uint64_t, whereas production code only uses byte spans.
If you really want to directly test `Serialize` uses `m_rotations[0]` and `Un
...
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2211078898)
Added to followup
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2211078898)
Added to followup
💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211080401)
Just a nit, anything is fine here, as it is optimized out anyway.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2211080401)
Just a nit, anything is fine here, as it is optimized out anyway.