💬 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.
💬 ryanofsky commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1719929110)
In commit "Add waitTipChanged to Mining interface" (d1c197fbbad86e3f966d9bf9abe4dd0aa76bc83d)
Not sure "in case of race condition" actually explains this. Would suggest changing this to:
- `current_tip` - block hash compared against the current chain tip. Function waits for the chain tip to change if this matches, otherwise it returns right away.
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1719929110)
In commit "Add waitTipChanged to Mining interface" (d1c197fbbad86e3f966d9bf9abe4dd0aa76bc83d)
Not sure "in case of race condition" actually explains this. Would suggest changing this to:
- `current_tip` - block hash compared against the current chain tip. Function waits for the chain tip to change if this matches, otherwise it returns right away.
💬 ryanofsky commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1719963458)
In commit "Add waitTipChanged to Mining interface" (d1c197fbbad86e3f966d9bf9abe4dd0aa76bc83d)
I think this comment is only warning about a small part of a bigger danger. It's not just unsafe to obtain the height here but unsafe to acquire cs_main anywhere in this code block, even after the while loop.
Would suggest dropping this comment and instead adding a simpler comment above LOCK(::cs_main) that says
"Must release g_best_block_mutex before locking cs_main, to avoid deadlocks."
A c
...
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1719963458)
In commit "Add waitTipChanged to Mining interface" (d1c197fbbad86e3f966d9bf9abe4dd0aa76bc83d)
I think this comment is only warning about a small part of a bigger danger. It's not just unsafe to obtain the height here but unsafe to acquire cs_main anywhere in this code block, even after the while loop.
Would suggest dropping this comment and instead adding a simpler comment above LOCK(::cs_main) that says
"Must release g_best_block_mutex before locking cs_main, to avoid deadlocks."
A c
...
💬 ryanofsky commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1719972779)
In commit "Add waitTipChanged to Mining interface" (d1c197fbbad86e3f966d9bf9abe4dd0aa76bc83d)
Not important but I think it would be clearer to condense these 3 lines to just:
```c++
g_best_block_cv.wait_until(lock, std::min(deadline, now + tick));
```
I don't think it helps to introduce 2 extra variables and subtract `now` then add `now` back.
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1719972779)
In commit "Add waitTipChanged to Mining interface" (d1c197fbbad86e3f966d9bf9abe4dd0aa76bc83d)
Not important but I think it would be clearer to condense these 3 lines to just:
```c++
g_best_block_cv.wait_until(lock, std::min(deadline, now + tick));
```
I don't think it helps to introduce 2 extra variables and subtract `now` then add `now` back.