📝 brunoerg converted_to_draft a pull request: "fuzz: wallet: add target for tx scanning"
(https://github.com/bitcoin/bitcoin/pull/32993)
Tracking issue (#29901)
This PR adds a fuzz target for wallet tx scanning (`ScanForWalletTransactions`). Unfortunately, it's not a regression test for the issue #31474 since it would require a more complex scenario/async operations.
(https://github.com/bitcoin/bitcoin/pull/32993)
Tracking issue (#29901)
This PR adds a fuzz target for wallet tx scanning (`ScanForWalletTransactions`). Unfortunately, it's not a regression test for the issue #31474 since it would require a more complex scenario/async operations.
💬 murchandamus commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3079233470)
> > @BitcoinMechanic:
> > > > The most persuasive argument for this would be that miners have diverged from defaults
> > >
> > > You continue to confuse cause and effect. Miners mine what gets relayed, not the reverse.
> > > Default relay policy for Core essentially controls what ends up in the blockchain and what doesn't. It's pointless to keep pretending otherwise.
> >
> > Neither Bitcoin Core, nor Knots, not even [LibreRelay nodes](https://github.com/petertodd/bitcoin/blob/af41065cb2a
...
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3079233470)
> > @BitcoinMechanic:
> > > > The most persuasive argument for this would be that miners have diverged from defaults
> > >
> > > You continue to confuse cause and effect. Miners mine what gets relayed, not the reverse.
> > > Default relay policy for Core essentially controls what ends up in the blockchain and what doesn't. It's pointless to keep pretending otherwise.
> >
> > Neither Bitcoin Core, nor Knots, not even [LibreRelay nodes](https://github.com/petertodd/bitcoin/blob/af41065cb2a
...
💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2210830294)
this fixed the error, thanks!
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2210830294)
this fixed the error, thanks!
⚠️ fanquake opened an issue: "fuzz: AddressSanitizer: odr-violation typeinfo name for CCoinsViewBacked"
(https://github.com/bitcoin/bitcoin/issues/32995)
```
# clang++ --version
clang version 20.1.8 (Fedora 20.1.8-1.fc43)
Target: aarch64-redhat-linux-gnu
make -C depends/ NO_QT=1 NO_ZMQ=1 NO_USDT=1 NO_WALLET=1 AR=llvm-ar NM=llvm-nm RANLIB=llvm-ranlib STRIP=llvm-strip
cmake -B build --toolchain /root/bitcoin/depends/aarch64-unknown-linux-gnu/toolchain.cmake -DBUILD_FOR_FUZZING=ON -DSANITIZERS=address -DAPPEND_CFLAGS="-flto=full" -DAPPEND_CXXFLAGS="-flto=full" -DAPPEND_LDFLAGS="-flto=full"
cmake --build build
```
```
./build/bin/fuzz
==============
...
(https://github.com/bitcoin/bitcoin/issues/32995)
```
# clang++ --version
clang version 20.1.8 (Fedora 20.1.8-1.fc43)
Target: aarch64-redhat-linux-gnu
make -C depends/ NO_QT=1 NO_ZMQ=1 NO_USDT=1 NO_WALLET=1 AR=llvm-ar NM=llvm-nm RANLIB=llvm-ranlib STRIP=llvm-strip
cmake -B build --toolchain /root/bitcoin/depends/aarch64-unknown-linux-gnu/toolchain.cmake -DBUILD_FOR_FUZZING=ON -DSANITIZERS=address -DAPPEND_CFLAGS="-flto=full" -DAPPEND_CXXFLAGS="-flto=full" -DAPPEND_LDFLAGS="-flto=full"
cmake --build build
```
```
./build/bin/fuzz
==============
...
💬 pablomartin4btc commented on pull request "wallet: Remove `upgradewallet` RPC":
(https://github.com/bitcoin/bitcoin/pull/32944#issuecomment-3079267669)
> migrated wallets will take over the legacy bdb version, which I am not sure is meaningful or useful. The upgradewallet RPC can be used to bump it.
As discussed offline with @achow101 a couple of days ago, my understanding was that both version field and ofc the `upgradewallet` RPC were exclusive to legacy wallets. I think the new migrated wallet has the latest version of features (`walletInstance->SetMinVersion(FEATURE_LATEST)`) as descriptors support everything, so they don't need to be bu
...
(https://github.com/bitcoin/bitcoin/pull/32944#issuecomment-3079267669)
> migrated wallets will take over the legacy bdb version, which I am not sure is meaningful or useful. The upgradewallet RPC can be used to bump it.
As discussed offline with @achow101 a couple of days ago, my understanding was that both version field and ofc the `upgradewallet` RPC were exclusive to legacy wallets. I think the new migrated wallet has the latest version of features (`walletInstance->SetMinVersion(FEATURE_LATEST)`) as descriptors support everything, so they don't need to be bu
...
💬 BitcoinMechanic commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3079331769)
>your assessment that "default relay policy for Core essentially controls what ends up in the blockchain and what doesn't" is obviously falsified.
It obviously remains true, you're just cherry picking. Again, the vast, overwhelming, super-super majority of what you see in the chain is what nodes will typically relay due to whatever Core's defaults are.
This is not an "overly general" statement, it's a fact that - I'm sorry to have to point out - is being ignored because that is convenient
...
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3079331769)
>your assessment that "default relay policy for Core essentially controls what ends up in the blockchain and what doesn't" is obviously falsified.
It obviously remains true, you're just cherry picking. Again, the vast, overwhelming, super-super majority of what you see in the chain is what nodes will typically relay due to whatever Core's defaults are.
This is not an "overly general" statement, it's a fact that - I'm sorry to have to point out - is being ignored because that is convenient
...
💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2210851772)
nit in 7b93fa394acaee7a715fae9a2b968d33c5adc174: Is the `min` needed? `misalign` is less than KEY_SIZE, and `target.size() > KEY_SIZE`.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2210851772)
nit in 7b93fa394acaee7a715fae9a2b968d33c5adc174: Is the `min` needed? `misalign` is less than KEY_SIZE, and `target.size() > KEY_SIZE`.
💬 maflcko commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2210500694)
nit in ddba3f5866868a6a27cd945508091439c6a82416: I think `XorWord` already explains the processing of multiple bytes at once, so this comment could be removed.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2210500694)
nit in ddba3f5866868a6a27cd945508091439c6a82416: I think `XorWord` already explains the processing of multiple bytes at once, so this comment could be removed.
💬 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?
(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
...
(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
...
(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.
(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.
(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
...
(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
...
(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.