💬 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.
💬 furszy commented on issue "wallet: setting changes are subject to race conditions":
(https://github.com/bitcoin/bitcoin/issues/30620#issuecomment-2293710537)
@ismaelsadeeq, the race is one level above.
`AddWalletSetting` obtains a copy of the 'wallet' settings json array, modifies it without any lock, and then calls `updateRwSetting`, which entirely replaces the existing value. If two threads access `AddWalletSetting` simultaneously, they both obtain the same 'wallet' settings array, append their new wallet, and store it. Only the data from the thread that executes the write operation last survives.
The same issue occurs on `RemoveWalletSetting`. T
...
(https://github.com/bitcoin/bitcoin/issues/30620#issuecomment-2293710537)
@ismaelsadeeq, the race is one level above.
`AddWalletSetting` obtains a copy of the 'wallet' settings json array, modifies it without any lock, and then calls `updateRwSetting`, which entirely replaces the existing value. If two threads access `AddWalletSetting` simultaneously, they both obtain the same 'wallet' settings array, append their new wallet, and store it. Only the data from the thread that executes the write operation last survives.
The same issue occurs on `RemoveWalletSetting`. T
...
✅ glozow closed an issue: "Follow-up ideas for XOR blocksdir *.dat files"
(https://github.com/bitcoin/bitcoin/issues/30599)
(https://github.com/bitcoin/bitcoin/issues/30599)
🚀 glozow merged a pull request: "test: add functional test for XORed block/undo files (`-blocksxor` option)"
(https://github.com/bitcoin/bitcoin/pull/30657)
(https://github.com/bitcoin/bitcoin/pull/30657)
💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1719998377)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1719998377)
Fixed.
👍 ryanofsky approved a pull request: "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals"
(https://github.com/bitcoin/bitcoin/pull/30409#pullrequestreview-2242860642)
Code review ACK 9ba60b3a2c22dcd1c9e7b4880f0adb678e4e8d63. Just fixed a few bugs to make waiting more reliable since last review and changed test logging. I like the new change to check for overflows generally in waitTipChanged, instead of just checking specifically for the max timeout value. That was better than the suggestion.
(https://github.com/bitcoin/bitcoin/pull/30409#pullrequestreview-2242860642)
Code review ACK 9ba60b3a2c22dcd1c9e7b4880f0adb678e4e8d63. Just fixed a few bugs to make waiting more reliable since last review and changed test logging. I like the new change to check for overflows generally in waitTipChanged, instead of just checking specifically for the max timeout value. That was better than the suggestion.
💬 ryanofsky commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1719939574)
In commit "Add waitTipChanged to Mining interface" (d1c197fbbad86e3f966d9bf9abe4dd0aa76bc83d)
Think it would be simpler to say "returns hash and height of the current chain tip after this call."
It seems potentially misleading to refer to a "previous" tip here because that sounds like this could sometimes return an old block hash. And it's a little confusing to refer to a "new "tip in the case where this function just returns immediately and there no interruption or wait or timeout.
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1719939574)
In commit "Add waitTipChanged to Mining interface" (d1c197fbbad86e3f966d9bf9abe4dd0aa76bc83d)
Think it would be simpler to say "returns hash and height of the current chain tip after this call."
It seems potentially misleading to refer to a "previous" tip here because that sounds like this could sometimes return an old block hash. And it's a little confusing to refer to a "new "tip in the case where this function just returns immediately and there no interruption or wait or timeout.