💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2210064078)
nit in 43189b844a95422fda6d14d4723035e6fc342aff: Any reason to create a copy now? I understand that in this commit the size is small enough to prefer pass-by-value, but this may not hold in the future?

💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2210137162)
nit in https://github.com/bitcoin/bitcoin/commit/43189b844a95422fda6d14d4723035e6fc342aff: Is exposing this implementation detail really needed? This is adding more lines to real code than there are test-only lines that need it. There are two tests:

* One that checks the `ToKey` round-trip. However, this is already checked by the existing tests and the serialize test can instead do the comparison on a `DataStream`, if needed: Check that a std::vector (de)serialization (roundtrip) is equal to
...

🤔 maflcko reviewed a pull request: "[IBD] multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-3024407006)
lgtm ack 7b93fa394acaee7a715fae9a2b968d33c5adc174 📕

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ack 7b93fa394acaee7a
...

💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2210191391)
nit in 43189b844a95422fda6d14d4723035e6fc342aff: obfuscation-serialization and vector-serialization is the same, so why is this changed? Leaving this as-is would avoid the need to introduce a separate `key_bytes` and then check the size again of `key_bytes` afterward.

💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2210898749)
Hmmm my `msan` ci job is not working (due to something unrelated) for me on my main dev machine, and `tsan` is segfaulting with `vm.mmap_rnd_bits=32`, so I'm going to leave this in one way or another for now.

💬 romanz commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2210901843)
I suggest saving the transaction position on disk (without its block offset):
```c++
bool TxoSpenderIndex::CustomAppend(const interfaces::BlockInfo& block)
{
std::vector<std::pair<COutPoint, CDiskTxPos>> items;
items.reserve(block.data->vtx.size());

FlatFilePos pos{block.file_number, block.data_pos};
pos.nPos += GetSizeOfCompactSize(block.data->vtx.size());
for (const auto& tx : block.data->vtx) {
if (!tx->IsCoinBase()) {
for (const auto& input
...

💬 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
...

💬 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...

💬 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.

💬 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
...

💬 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.

💬 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.

💬 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
...

💬 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.

💬 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?

💬 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.

💬 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.

💬 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.

💬 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.

💬 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?