🤔 stickies-v reviewed a pull request: "Adds transaction propagation information to mempool transactions"
(https://github.com/bitcoin/bitcoin/pull/32986#pullrequestreview-3025461492)
I'm generally not in favour of making bespoke changes to production code or extending the public interface just to facilitate tests, monitoring, ... if there exists a workaround, especially for temporary projects. In this case, after speaking with @sr-gi offline a bit more, it seems it seems like tracepoints, logging, small patchsets, ... are feasible alternatives, so I'm Concept NACK on this one. (but I am open to making minimal changes to ensure the necessary erlay simulations can be ran)
(https://github.com/bitcoin/bitcoin/pull/32986#pullrequestreview-3025461492)
I'm generally not in favour of making bespoke changes to production code or extending the public interface just to facilitate tests, monitoring, ... if there exists a workaround, especially for temporary projects. In this case, after speaking with @sr-gi offline a bit more, it seems it seems like tracepoints, logging, small patchsets, ... are feasible alternatives, so I'm Concept NACK on this one. (but I am open to making minimal changes to ensure the necessary erlay simulations can be ran)
🤔 stickies-v reviewed a pull request: "doc: clarify GetAddresses documentation"
(https://github.com/bitcoin/bitcoin/pull/32994#pullrequestreview-3025490230)
Concept ACK. I would strongly prefer to go further and rename the trusted function to `GetAddressesUncached`. Overloads that behave vastly differently and can lead to dangerous behaviour is a big red flag for me.
(https://github.com/bitcoin/bitcoin/pull/32994#pullrequestreview-3025490230)
Concept ACK. I would strongly prefer to go further and rename the trusted function to `GetAddressesUncached`. Overloads that behave vastly differently and can lead to dangerous behaviour is a big red flag for me.
🤔 theuni reviewed a pull request: "depends: fix libevent `_WIN32_WINNT` usage"
(https://github.com/bitcoin/bitcoin/pull/32837#pullrequestreview-3025518171)
Concept ACK and vague post-merge utACK.
I don't understand all of the upstream changes, but it makes sense to take that patch if it fixes our issue.
(https://github.com/bitcoin/bitcoin/pull/32837#pullrequestreview-3025518171)
Concept ACK and vague post-merge utACK.
I don't understand all of the upstream changes, but it makes sense to take that patch if it fixes our issue.
💬 theuni commented on pull request "depends: fix libevent `_WIN32_WINNT` usage":
(https://github.com/bitcoin/bitcoin/pull/32837#discussion_r2210729180)
Why was this undef'd before and not now?
(https://github.com/bitcoin/bitcoin/pull/32837#discussion_r2210729180)
Why was this undef'd before and not now?
💬 fanquake commented on pull request "depends: fix libevent `_WIN32_WINNT` usage":
(https://github.com/bitcoin/bitcoin/pull/32837#discussion_r2210782699)
The upstream change just removed all the undefining: https://github.com/libevent/libevent/pull/1498.
(https://github.com/bitcoin/bitcoin/pull/32837#discussion_r2210782699)
The upstream change just removed all the undefining: https://github.com/libevent/libevent/pull/1498.
💬 fanquake commented on pull request "fuzz: wallet: add target for tx scanning":
(https://github.com/bitcoin/bitcoin/pull/32993#issuecomment-3079192425)
https://cirrus-ci.com/task/4897452429410304?logs=ci#L6267:
```bash
[10:48:30.374] Run wallet_scan with args ['/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/bin/fuzz', '-max_total_time=60']INFO: Running with entropic power schedule (0xFF, 100).
[10:48:30.374] INFO: Seed: 1927962085
[10:48:30.374] INFO: Loaded 1 modules (634026 inline 8-bit counters): 634026 [0x5573df666fb8, 0x5573df701c62),
[10:48:30.374] INFO: Loaded 1 PC tables (634026 PCs): 634026 [0x5573df701c68,0x5573e00ae7
...
(https://github.com/bitcoin/bitcoin/pull/32993#issuecomment-3079192425)
https://cirrus-ci.com/task/4897452429410304?logs=ci#L6267:
```bash
[10:48:30.374] Run wallet_scan with args ['/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/bin/fuzz', '-max_total_time=60']INFO: Running with entropic power schedule (0xFF, 100).
[10:48:30.374] INFO: Seed: 1927962085
[10:48:30.374] INFO: Loaded 1 modules (634026 inline 8-bit counters): 634026 [0x5573df666fb8, 0x5573df701c62),
[10:48:30.374] INFO: Loaded 1 PC tables (634026 PCs): 634026 [0x5573df701c68,0x5573e00ae7
...
📝 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
...