💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719761131)
Continued in https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718988155
Resolving this one for now.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719761131)
Continued in https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1718988155
Resolving this one for now.
💬 fjahr commented on pull request "kernel: pre-28.x chainparams and headerssync update":
(https://github.com/bitcoin/bitcoin/pull/30658#issuecomment-2293446211)
ACK cd913de6488d25057e3bf7dcad1508e9d689f87f
Aside from the size-on-disk weirdness discussed above, I have compared all the values to my local nodes in the different networks and got matches each time. I also executed `contrib/devtools/headerssync-params.py` locally and got the same results.
(https://github.com/bitcoin/bitcoin/pull/30658#issuecomment-2293446211)
ACK cd913de6488d25057e3bf7dcad1508e9d689f87f
Aside from the size-on-disk weirdness discussed above, I have compared all the values to my local nodes in the different networks and got matches each time. I also executed `contrib/devtools/headerssync-params.py` locally and got the same results.
👍 TheCharlatan approved a pull request: "fuzz: Faster utxo_snapshot fuzz target"
(https://github.com/bitcoin/bitcoin/pull/30644#pullrequestreview-2242642075)
Re-ACK fa0b66400c12d11579b38b1ac76f677b23e8c82a
I saw the old "libFuzzer disabled leak detection" info message in the invalid test, but not sure what might be causing it.
(https://github.com/bitcoin/bitcoin/pull/30644#pullrequestreview-2242642075)
Re-ACK fa0b66400c12d11579b38b1ac76f677b23e8c82a
I saw the old "libFuzzer disabled leak detection" info message in the invalid test, but not sure what might be causing it.
💬 maflcko commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2293464039)
> I saw the old "libFuzzer disabled leak detection" info message in the invalid test, but not sure what might be causing it.
I haven't seen that one. Are you saying it only happens for this test? What are the steps to reproduce?
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2293464039)
> I saw the old "libFuzzer disabled leak detection" info message in the invalid test, but not sure what might be causing it.
I haven't seen that one. Are you saying it only happens for this test? What are the steps to reproduce?
💬 sipa commented on pull request "seeds: Pull additional nodes from my seeder and update fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/30008#discussion_r1719813387)
The file is from May 2022. No reason to keep using it; there is no inherent reason why it should be considered more reliable than yours.
(https://github.com/bitcoin/bitcoin/pull/30008#discussion_r1719813387)
The file is from May 2022. No reason to keep using it; there is no inherent reason why it should be considered more reliable than yours.
💬 TheCharlatan commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2293495649)
> I haven't seen that one. Are you saying it only happens for this test? What are the steps to reproduce?
Only saw it for the invalid test after about 2 million iterations just running `FUZZ=utxo_snapshot_invalid ./src/test/fuzz/fuzz`, configured with `address,fuzzer` (though I guess that should not matter).
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2293495649)
> I haven't seen that one. Are you saying it only happens for this test? What are the steps to reproduce?
Only saw it for the invalid test after about 2 million iterations just running `FUZZ=utxo_snapshot_invalid ./src/test/fuzz/fuzz`, configured with `address,fuzzer` (though I guess that should not matter).
💬 glozow commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2293497129)
fwiw, my bench results (I commented out `assert(iters_performed == max_iters)` since it fails)
`git rebase -i --exec="make -j12 > /dev/null && src/bench/bench_bitcoin -filter=Linearize.*" HEAD~11`
Output: [bench_30286_all.txt](https://github.com/user-attachments/files/16637507/bench_30286_all.txt)
Reformatted as a csv: [output_benchmarks_ns_op.csv](https://github.com/user-attachments/files/16637477/output_benchmarks_ns_op.csv)
Displayed as a line graph (x-axis is commit, y-axis is ns
...
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2293497129)
fwiw, my bench results (I commented out `assert(iters_performed == max_iters)` since it fails)
`git rebase -i --exec="make -j12 > /dev/null && src/bench/bench_bitcoin -filter=Linearize.*" HEAD~11`
Output: [bench_30286_all.txt](https://github.com/user-attachments/files/16637507/bench_30286_all.txt)
Reformatted as a csv: [output_benchmarks_ns_op.csv](https://github.com/user-attachments/files/16637477/output_benchmarks_ns_op.csv)
Displayed as a line graph (x-axis is commit, y-axis is ns
...
👍 hodlinator approved a pull request: "refactor: Allow CScript construction from any std::input_iterator"
(https://github.com/bitcoin/bitcoin/pull/29369#pullrequestreview-2242723589)
ACK fa7b9b99a2ed466d1765d748f4a59c2f581b974f
Verified that it resolved the issue I was having on other PR (https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719107583) through cherry-picking the commits in this PR, undoing the local workaround, and pushing to a personal GitHub branch 239709e194203e06bd1f0610059e00998f645d4c to see if Windows CI compiled it successfully, which it did (https://github.com/hodlinator/bitcoin/actions/runs/10419824929/job/28858657327).
nit: I'd prefer t
...
(https://github.com/bitcoin/bitcoin/pull/29369#pullrequestreview-2242723589)
ACK fa7b9b99a2ed466d1765d748f4a59c2f581b974f
Verified that it resolved the issue I was having on other PR (https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1719107583) through cherry-picking the commits in this PR, undoing the local workaround, and pushing to a personal GitHub branch 239709e194203e06bd1f0610059e00998f645d4c to see if Windows CI compiled it successfully, which it did (https://github.com/hodlinator/bitcoin/actions/runs/10419824929/job/28858657327).
nit: I'd prefer t
...
📝 maflcko opened a pull request: "test: Enable detect_leaks=1 in ASAN_OPTIONS explicitly"
(https://github.com/bitcoin/bitcoin/pull/30665)
It should be enabled by default, but being explicit can't hurt.
(https://github.com/bitcoin/bitcoin/pull/30665)
It should be enabled by default, but being explicit can't hurt.
💬 maflcko commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2293522814)
Lsan should be enabled by default, so if there was a problem, it should still be found. I've enabled it explicitly in https://github.com/bitcoin/bitcoin/pull/30665, but that should be unrelated to the changes here.
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2293522814)
Lsan should be enabled by default, so if there was a problem, it should still be found. I've enabled it explicitly in https://github.com/bitcoin/bitcoin/pull/30665, but that should be unrelated to the changes here.
💬 0xB10C commented on pull request "kernel: pre-28.x chainparams and headerssync update":
(https://github.com/bitcoin/bitcoin/pull/30658#discussion_r1719854952)
```
$ du -sh blocks/ chainstate/
84G blocks/
13G chainstate/
```
(https://github.com/bitcoin/bitcoin/pull/30658#discussion_r1719854952)
```
$ du -sh blocks/ chainstate/
84G blocks/
13G chainstate/
```
💬 maflcko commented on pull request "refactor: Allow CScript construction from any std::input_iterator":
(https://github.com/bitcoin/bitcoin/pull/29369#issuecomment-2293532880)
> changes were squashed into a second commit as they appear to belong closer together.
Yeah, they are related, but the prevector one adds requirements on the code, and the cscript one removes requirements on the code, which is why they are in two commits.
(https://github.com/bitcoin/bitcoin/pull/29369#issuecomment-2293532880)
> changes were squashed into a second commit as they appear to belong closer together.
Yeah, they are related, but the prevector one adds requirements on the code, and the cscript one removes requirements on the code, which is why they are in two commits.
💬 glozow commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1719919217)
I tried this on my apple machine and am able to make this test fail if I add a sleep right before the line checking `fs::is_empty`. No sign of DS_Store. It kind of seems more like blk0000.dat, rev0000.dat, and index/ are showing up early... I don't think I know init well enough to know exactly what order everything should happen in.
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1719919217)
I tried this on my apple machine and am able to make this test fail if I add a sleep right before the line checking `fs::is_empty`. No sign of DS_Store. It kind of seems more like blk0000.dat, rev0000.dat, and index/ are showing up early... I don't think I know init well enough to know exactly what order everything should happen in.
💬 maflcko commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1719948326)
Thanks for checking!
I honestly don't known if this should be considered a bug in Bitcoin Core. If so, are we going to enumerate all external programs that fiddle with the blocksdir without permission and add workarounds for them?
To me it seems like this is something that Apple should fix, or Apple users (by not using Apple, or by adjusting the Finder settings to leave Bitcoin Core alone).
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1719948326)
Thanks for checking!
I honestly don't known if this should be considered a bug in Bitcoin Core. If so, are we going to enumerate all external programs that fiddle with the blocksdir without permission and add workarounds for them?
To me it seems like this is something that Apple should fix, or Apple users (by not using Apple, or by adjusting the Finder settings to leave Bitcoin Core alone).
💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1719949119)
@glozow Cool, building on that:
```
$ git rebase --exec='F=$(mktemp) && make -j -C src bench/bench_bitcoin >/dev/null && src/bench/bench_bitcoin -filter=Linearize64TxWorstCase.* -min-time=60000 -output-json=$F >/dev/null 2>/dev/null && NS=$(cat $F | jq "(.results[0][\"median(elapsed)\"] - .results[1][\"median(elapsed)\"]) / 10000 * 1000000000") && TITLE=$(git show -s --format="%s") && echo "$NS $TITLE"' HEAD~11 2>/dev/null
11.60165442504903 clusterlin bench: have low/high iter benchmarks i
...
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1719949119)
@glozow Cool, building on that:
```
$ git rebase --exec='F=$(mktemp) && make -j -C src bench/bench_bitcoin >/dev/null && src/bench/bench_bitcoin -filter=Linearize64TxWorstCase.* -min-time=60000 -output-json=$F >/dev/null 2>/dev/null && NS=$(cat $F | jq "(.results[0][\"median(elapsed)\"] - .results[1][\"median(elapsed)\"]) / 10000 * 1000000000") && TITLE=$(git show -s --format="%s") && echo "$NS $TITLE"' HEAD~11 2>/dev/null
11.60165442504903 clusterlin bench: have low/high iter benchmarks i
...
💬 alfonsoromanz commented on pull request "test: assumeutxo: add missing tests in wallet_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1719951882)
Hey @fjahr! Thank you for taking the time to review this. I haven't had much time to work on it lately, but I do recall that during my debugging, I found the `rescan_height` was 199 [at this point ](https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.cpp#L3311), while the `block_height` was 299 (the oldest block stored on disk), hence triggering the error mentioned.
The `rescan_height` of 199 seems to match the wallet's birthday, given that the wallet was created at the beginning
...
(https://github.com/bitcoin/bitcoin/pull/30455#discussion_r1719951882)
Hey @fjahr! Thank you for taking the time to review this. I haven't had much time to work on it lately, but I do recall that during my debugging, I found the `rescan_height` was 199 [at this point ](https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.cpp#L3311), while the `block_height` was 299 (the oldest block stored on disk), hence triggering the error mentioned.
The `rescan_height` of 199 seems to match the wallet's birthday, given that the wallet was created at the beginning
...
💬 maflcko commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1719955094)
I guess `fs::is_empty(opts.blocks_dir)` could be replaced by `not fs::exists("blk0000.dat")`?
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1719955094)
I guess `fs::is_empty(opts.blocks_dir)` could be replaced by `not fs::exists("blk0000.dat")`?
💬 achow101 commented on pull request "kernel: pre-28.x chainparams and headerssync update":
(https://github.com/bitcoin/bitcoin/pull/30658#discussion_r1719991493)
I'm quite confused as to how all of you have way larger blocks and chainstate dirs than I do. I checked my 3 nodes and they all agree on these sizes, and all 3 have very high uptimes.
Anyways, bumped `m_assumed_blockchain_size` to 93 and `m_assumed_chain_state_size` to 19, which I think should cover all sizes given here.
(https://github.com/bitcoin/bitcoin/pull/30658#discussion_r1719991493)
I'm quite confused as to how all of you have way larger blocks and chainstate dirs than I do. I checked my 3 nodes and they all agree on these sizes, and all 3 have very high uptimes.
Anyways, bumped `m_assumed_blockchain_size` to 93 and `m_assumed_chain_state_size` to 19, which I think should cover all sizes given here.
💬 achow101 commented on pull request "kernel: pre-28.x chainparams and headerssync update":
(https://github.com/bitcoin/bitcoin/pull/30658#issuecomment-2293710417)
> If you have to touch, can you make `contrib/devtools/headerssync-params.py` executable?
Done
(https://github.com/bitcoin/bitcoin/pull/30658#issuecomment-2293710417)
> If you have to touch, can you make `contrib/devtools/headerssync-params.py` executable?
Done
💬 glozow commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1719992797)
fwiw the sleepy version doesn't seem to use 0 as xor key when I start a regtest chain normally, so maybe your guess about temp directory paranormal activity is correct.
Going to merge this test since there's not a problem with it.
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1719992797)
fwiw the sleepy version doesn't seem to use 0 as xor key when I start a regtest chain normally, so maybe your guess about temp directory paranormal activity is correct.
Going to merge this test since there's not a problem with it.