💬 rkrux commented on pull request "test: remove strict restrictions on rpc_deprecated test":
(https://github.com/bitcoin/bitcoin/pull/32139#discussion_r2014391916)
Interesting, I was wondering why, ended up tracking down the conditional, makes sense now.
https://github.com/bitcoin/bitcoin/blob/c0b7159de47592c95c6db061de682b4af4058102/test/functional/test_framework/test_framework.py#L470-L471
which is due to:
https://github.com/bitcoin/bitcoin/blob/c0b7159de47592c95c6db061de682b4af4058102/test/functional/test_framework/test_framework.py#L158-L160
(https://github.com/bitcoin/bitcoin/pull/32139#discussion_r2014391916)
Interesting, I was wondering why, ended up tracking down the conditional, makes sense now.
https://github.com/bitcoin/bitcoin/blob/c0b7159de47592c95c6db061de682b4af4058102/test/functional/test_framework/test_framework.py#L470-L471
which is due to:
https://github.com/bitcoin/bitcoin/blob/c0b7159de47592c95c6db061de682b4af4058102/test/functional/test_framework/test_framework.py#L158-L160
💬 yancyribbens commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2014404157)
Thanks. It's sort of interesting that this sort of thing doesn't even appear in the Github review. This implies that if one only reviews by Github diff, there is hidden characters that can go in un-noticed. Presumably in this case, there is some editors that can translated this UTF-8 encoding.
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2014404157)
Thanks. It's sort of interesting that this sort of thing doesn't even appear in the Github review. This implies that if one only reviews by Github diff, there is hidden characters that can go in un-noticed. Presumably in this case, there is some editors that can translated this UTF-8 encoding.
👍 ryanofsky approved a pull request: "qa: Verify clean shutdown on startup failure"
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2717633497)
Code review ACK 6539598b42005f453b358a0076287db99d1b02eb. Since last review just simplified errno code a little as suggested
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2717633497)
Code review ACK 6539598b42005f453b358a0076287db99d1b02eb. Since last review just simplified errno code a little as suggested
💬 rkrux commented on pull request "test: remove strict restrictions on rpc_deprecated test":
(https://github.com/bitcoin/bitcoin/pull/32139#discussion_r2014415397)
The `skip_if_no_wallet` function updates the internal state of the test framework, which I wouldn't have guessed just by its name.
(https://github.com/bitcoin/bitcoin/pull/32139#discussion_r2014415397)
The `skip_if_no_wallet` function updates the internal state of the test framework, which I wouldn't have guessed just by its name.
👍 maflcko approved a pull request: "test: Add functional test for bitcoin-chainstate"
(https://github.com/bitcoin/bitcoin/pull/32145#pullrequestreview-2717656967)
ACK ca55613fd1596c3e9e5c3cc5c4a7ea4591841fc0 🎭
<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: ACK ca55613fd1596c3e9e5c3cc5c4
...
(https://github.com/bitcoin/bitcoin/pull/32145#pullrequestreview-2717656967)
ACK ca55613fd1596c3e9e5c3cc5c4a7ea4591841fc0 🎭
<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: ACK ca55613fd1596c3e9e5c3cc5c4
...
💬 maflcko commented on pull request "test: Add functional test for bitcoin-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32145#discussion_r2014419548)
could just use assert_equal?
(https://github.com/bitcoin/bitcoin/pull/32145#discussion_r2014419548)
could just use assert_equal?
💬 l0rinc commented on pull request "[IBD] flush UTXOs in bigger batches":
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2754814130)
Thanks for your measurements @Eunovo!
I'm not sure if we support 1GiB of total memory or not, but [even 8 years ago](https://github.com/bitcoin/bitcoin/issues/11752#issuecomment-346451450) that required a lower dbcache that 450MiB.
It took me a bit, but I finally finished comparing the 16 and 64 MiB batches with `valgrind --tool=massif` for full IBD until block 888888.
<details>
<summary>Benchmarks</summary>
```bash
COMMITS="998386d4462f5e06412303ba559791da83b913fb 5cefc4bbadaa06f2
...
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2754814130)
Thanks for your measurements @Eunovo!
I'm not sure if we support 1GiB of total memory or not, but [even 8 years ago](https://github.com/bitcoin/bitcoin/issues/11752#issuecomment-346451450) that required a lower dbcache that 450MiB.
It took me a bit, but I finally finished comparing the 16 and 64 MiB batches with `valgrind --tool=massif` for full IBD until block 888888.
<details>
<summary>Benchmarks</summary>
```bash
COMMITS="998386d4462f5e06412303ba559791da83b913fb 5cefc4bbadaa06f2
...
💬 maflcko commented on pull request "test: remove strict restrictions on rpc_deprecated test":
(https://github.com/bitcoin/bitcoin/pull/32139#discussion_r2014425142)
I guess it could make sense to require explicit setup of any wallets in tests, but this can be done in a follow-up, probably best after the bdb removal
(https://github.com/bitcoin/bitcoin/pull/32139#discussion_r2014425142)
I guess it could make sense to require explicit setup of any wallets in tests, but this can be done in a follow-up, probably best after the bdb removal
💬 maflcko commented on issue "callgrind_annotate broken after cmake migration?":
(https://github.com/bitcoin/bitcoin/issues/31957#issuecomment-2754833699)
> I've tested the master branch @ [c0b7159](https://github.com/bitcoin/bitcoin/commit/c0b7159de47592c95c6db061de682b4af4058102) with the diff from [#31957 (comment)](https://github.com/bitcoin/bitcoin/issues/31957#issue-2882058716) on Ubuntu 24.04:
I can't reproduce. What are the exact steps to reproduce from a fresh install of the operating system, like in OP?
(https://github.com/bitcoin/bitcoin/issues/31957#issuecomment-2754833699)
> I've tested the master branch @ [c0b7159](https://github.com/bitcoin/bitcoin/commit/c0b7159de47592c95c6db061de682b4af4058102) with the diff from [#31957 (comment)](https://github.com/bitcoin/bitcoin/issues/31957#issue-2882058716) on Ubuntu 24.04:
I can't reproduce. What are the exact steps to reproduce from a fresh install of the operating system, like in OP?
⚠️ Kotarou10 opened an issue: "Accedence; SAI-15; SLH-DSA."
(https://github.com/bitcoin/bitcoin/issues/32147)
Will SAI-15 be a priority for Developers to run this? Let's look at it mathematically and this is Reference (https://syamailcointheogwhitepaper.tiiny.site) SAI-15 Key Generation and Encryption Framework
def generate_key(self):
return secrets.token_bytes(399)
def encrypt(self, plaintext, key):
if len(key) != 399:
raise ValueError("Key must be exactly 399 bytes")
ciphertext = bytearray()
for i, byte in enumerate(plaintext):
key_byte = key[i % 399]
ciphertex
...
(https://github.com/bitcoin/bitcoin/issues/32147)
Will SAI-15 be a priority for Developers to run this? Let's look at it mathematically and this is Reference (https://syamailcointheogwhitepaper.tiiny.site) SAI-15 Key Generation and Encryption Framework
def generate_key(self):
return secrets.token_bytes(399)
def encrypt(self, plaintext, key):
if len(key) != 399:
raise ValueError("Key must be exactly 399 bytes")
ciphertext = bytearray()
for i, byte in enumerate(plaintext):
key_byte = key[i % 399]
ciphertex
...
✅ pinheadmz closed an issue: "Accedence; SAI-15; SLH-DSA."
(https://github.com/bitcoin/bitcoin/issues/32147)
(https://github.com/bitcoin/bitcoin/issues/32147)
💬 pinheadmz commented on issue "Accedence; SAI-15; SLH-DSA.":
(https://github.com/bitcoin/bitcoin/issues/32147#issuecomment-2754840927)
I don't think this has anything to do with bitcoin so, closing.
(https://github.com/bitcoin/bitcoin/issues/32147#issuecomment-2754840927)
I don't think this has anything to do with bitcoin so, closing.
💬 marcofleon commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-2754900599)
I explored the instablilty of the `coins_db` target some more and it seems to be in LevelDB, so I think we can ignore it as an issue for this fuzz test.
I generated two coverage reports: [one](https://marcofleon.github.io/coverage/coinsdb_stable/) with a corpus of inputs that were all stable (afl++ showing 99.3%) and then [one](https://marcofleon.github.io/coverage/coinsdb_unstable/) with that same corpus but just a single added unstable input (afl++ showing ~75%). You can see that basically
...
(https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-2754900599)
I explored the instablilty of the `coins_db` target some more and it seems to be in LevelDB, so I think we can ignore it as an issue for this fuzz test.
I generated two coverage reports: [one](https://marcofleon.github.io/coverage/coinsdb_stable/) with a corpus of inputs that were all stable (afl++ showing 99.3%) and then [one](https://marcofleon.github.io/coverage/coinsdb_unstable/) with that same corpus but just a single added unstable input (afl++ showing ~75%). You can see that basically
...
💬 polespinasa commented on issue "Migrate from BTC/kvB to sat/vB on RPC and startup options":
(https://github.com/bitcoin/bitcoin/issues/32093#issuecomment-2754938881)
> my suggestion would be to allow three decimal precision if we decide to change RPCs to sat/vB.
That sounds good to me. I'm not sure though if we want to change RPCs to sat/vB or just add both options. Although some RPC calls are working only with sat/vB like `send` or `sendtomany`.
> It might also be possible to generally have more precision if we update internally to replace CFeeRate with FeeFrac.
That was one of the main points given by @ismaelsadeeq here https://github.com/bitcoin/bitcoi
...
(https://github.com/bitcoin/bitcoin/issues/32093#issuecomment-2754938881)
> my suggestion would be to allow three decimal precision if we decide to change RPCs to sat/vB.
That sounds good to me. I'm not sure though if we want to change RPCs to sat/vB or just add both options. Although some RPC calls are working only with sat/vB like `send` or `sendtomany`.
> It might also be possible to generally have more precision if we update internally to replace CFeeRate with FeeFrac.
That was one of the main points given by @ismaelsadeeq here https://github.com/bitcoin/bitcoi
...
💬 laanwj commented on pull request "contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-2754961477)
> git diff <(xxd rocky9.3/Xcode-15.0-15A240d-extracted-SDK-with-libcxx-headers.tar.gz) <(xxd fedora41/Xcode-15.0-15A240d-extracted-SDK-with-libcxx-headers.tar.gz)
This seems to suggest the divergence is in the tarring, not the gzipping. Do you still see a difference if you `gunzip` the .gzs and compare the tars only?
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-2754961477)
> git diff <(xxd rocky9.3/Xcode-15.0-15A240d-extracted-SDK-with-libcxx-headers.tar.gz) <(xxd fedora41/Xcode-15.0-15A240d-extracted-SDK-with-libcxx-headers.tar.gz)
This seems to suggest the divergence is in the tarring, not the gzipping. Do you still see a difference if you `gunzip` the .gzs and compare the tars only?
🤔 kevkevinpal reviewed a pull request: "test: Add functional test for bitcoin-chainstate"
(https://github.com/bitcoin/bitcoin/pull/32145#pullrequestreview-2717896921)
ACK ca55613fd1596c3e9e5c3cc5c4a7ea4591841fc0
looks good to me I think we can expand on the test, but not blocking this change
(https://github.com/bitcoin/bitcoin/pull/32145#pullrequestreview-2717896921)
ACK ca55613fd1596c3e9e5c3cc5c4a7ea4591841fc0
looks good to me I think we can expand on the test, but not blocking this change
💬 kevkevinpal commented on pull request "test: Add functional test for bitcoin-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32145#discussion_r2014560812)
not something that may block this PR but I see other error messages in `bitcoin-chainstate.cpp`
maybe can be addressed in a followup PR
```
grep -nri "std::cerr" ./src/bitcoin-chainstate.cpp
(manually removed the ones that are already in this PR)
53: std::cerr
54 << "Usage: " << argv[0] << " DATADIR" << std::endl
55 << "Display DATADIR information, and process hex-encoded blocks on standard input." << std::endl
56 << std::endl
...
(https://github.com/bitcoin/bitcoin/pull/32145#discussion_r2014560812)
not something that may block this PR but I see other error messages in `bitcoin-chainstate.cpp`
maybe can be addressed in a followup PR
```
grep -nri "std::cerr" ./src/bitcoin-chainstate.cpp
(manually removed the ones that are already in this PR)
53: std::cerr
54 << "Usage: " << argv[0] << " DATADIR" << std::endl
55 << "Display DATADIR information, and process hex-encoded blocks on standard input." << std::endl
56 << std::endl
...
💬 kevkevinpal commented on pull request "lint: Remove needless borrow to fix Clippy warning":
(https://github.com/bitcoin/bitcoin/pull/32144#issuecomment-2755037093)
ACK [e3ce2bd](https://github.com/bitcoin/bitcoin/pull/32144/commits/e3ce2bd9829bbe14e5da26505ac9d68ae0d2af2d)
lgtm, you might want to put that summary comment in the PR description since most would look there first to get a summary of the change
(https://github.com/bitcoin/bitcoin/pull/32144#issuecomment-2755037093)
ACK [e3ce2bd](https://github.com/bitcoin/bitcoin/pull/32144/commits/e3ce2bd9829bbe14e5da26505ac9d68ae0d2af2d)
lgtm, you might want to put that summary comment in the PR description since most would look there first to get a summary of the change
💬 brunoerg commented on pull request "doc: use `libfuzzer-nosan` for macOS":
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2755082776)
@fjahr Did you try with `ASAN_OPTIONS=detect_container_overflow=0`? If so, did you get any issue?
(https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-2755082776)
@fjahr Did you try with `ASAN_OPTIONS=detect_container_overflow=0`? If so, did you get any issue?
🤔 marcofleon reviewed a pull request: "fuzz: extract unsequenced operations with side-effects"
(https://github.com/bitcoin/bitcoin/pull/32141#pullrequestreview-2717999056)
Nice, ACK b1de59e8965354fff5a149bc0fe61ed0704aea7a
(https://github.com/bitcoin/bitcoin/pull/32141#pullrequestreview-2717999056)
Nice, ACK b1de59e8965354fff5a149bc0fe61ed0704aea7a