💬 maflcko commented on pull request "log: print reason when writing chainstate":
(https://github.com/bitcoin/bitcoin/pull/32404#discussion_r2072592135)
the function args are missing `std::common_type_t`, as explained in the function immediately above.
(https://github.com/bitcoin/bitcoin/pull/32404#discussion_r2072592135)
the function args are missing `std::common_type_t`, as explained in the function immediately above.
💬 maflcko commented on pull request "log: print reason when writing chainstate":
(https://github.com/bitcoin/bitcoin/pull/32404#discussion_r2072592239)
any reason to remove the trailing whitespace?
(https://github.com/bitcoin/bitcoin/pull/32404#discussion_r2072592239)
any reason to remove the trailing whitespace?
🤔 maflcko reviewed a pull request: "fuzz: Suppress abort message on Windows"
(https://github.com/bitcoin/bitcoin/pull/32409#pullrequestreview-2813444120)
No objection, but assert/Assert seems to be used widely in the codebase, so shouldn't this be done for all test binaries, or none? Otherwise the same assert could lead to inconsistent behavior, depending on which test binary ran into it.
(https://github.com/bitcoin/bitcoin/pull/32409#pullrequestreview-2813444120)
No objection, but assert/Assert seems to be used widely in the codebase, so shouldn't this be done for all test binaries, or none? Otherwise the same assert could lead to inconsistent behavior, depending on which test binary ran into it.
💬 maflcko commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2072606185)
https://cirrus-ci.com/task/5276299050090496?logs=ci#L3261:
```
[15:13:21.244] node2 2025-05-02T19:13:19.879966Z [httpworker.4] [rpc/request.cpp:241] [parse] [rpc] ThreadRPCServer method=getmempoolinfo user=__cookie__
[15:13:21.244] test 2025-05-02T19:13:19.882000Z TestFramework.node2 (DEBUG): RPC successfully started
[15:13:21.244] test 2025-05-02T19:13:20.885000Z TestFramework (ERROR): Assertion failed
[15:13:21.244] Traceback (most recent cal
...
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2072606185)
https://cirrus-ci.com/task/5276299050090496?logs=ci#L3261:
```
[15:13:21.244] node2 2025-05-02T19:13:19.879966Z [httpworker.4] [rpc/request.cpp:241] [parse] [rpc] ThreadRPCServer method=getmempoolinfo user=__cookie__
[15:13:21.244] test 2025-05-02T19:13:19.882000Z TestFramework.node2 (DEBUG): RPC successfully started
[15:13:21.244] test 2025-05-02T19:13:20.885000Z TestFramework (ERROR): Assertion failed
[15:13:21.244] Traceback (most recent cal
...
💬 pinheadmz commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2072612696)
yeah i see it -- gotta figure out why `sock.recv(1024)` is non blocking on the two failing platforms
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2072612696)
yeah i see it -- gotta figure out why `sock.recv(1024)` is non blocking on the two failing platforms
✅ maflcko closed a pull request: "Bugfix: Correct first-run free space checks"
(https://github.com/bitcoin/bitcoin/pull/29678)
(https://github.com/bitcoin/bitcoin/pull/29678)
💬 maflcko commented on pull request "Bugfix: Correct first-run free space checks":
(https://github.com/bitcoin/bitcoin/pull/29678#issuecomment-2849209362)
(close-open for fresh GitHub CI, I guess the Cirrus failure can be ignored for now)
(https://github.com/bitcoin/bitcoin/pull/29678#issuecomment-2849209362)
(close-open for fresh GitHub CI, I guess the Cirrus failure can be ignored for now)
📝 maflcko reopened a pull request: "Bugfix: Correct first-run free space checks"
(https://github.com/bitcoin/bitcoin/pull/29678)
It's not clear what `m_assumed_*_size` are actually set based on, but historically it was in GB, not GiB, and that's still used in the GUI which is more user-facing.
Could just as easily change the GUI if GiB is preferred.
(https://github.com/bitcoin/bitcoin/pull/29678)
It's not clear what `m_assumed_*_size` are actually set based on, but historically it was in GB, not GiB, and that's still used in the GUI which is more user-facing.
Could just as easily change the GUI if GiB is preferred.
💬 maflcko commented on pull request "[IBD] specialize block serialization":
(https://github.com/bitcoin/bitcoin/pull/31868#issuecomment-2849214120)
> > If IBD speedup is not the main motivation, what else is the motivation, given that the title mentions [IBD]?
>
> I think block serialization being fast could be useful for future kernel users. Reading and de-serializing blocks is one of the common operations I'd expect outside of just pure block validation.
Ok, seems fine. It would be good to list the main motivation(s) in the OP. Currently, it only says IBD and that IBD *isn't* the main motivation. However, I don't understand how this
...
(https://github.com/bitcoin/bitcoin/pull/31868#issuecomment-2849214120)
> > If IBD speedup is not the main motivation, what else is the motivation, given that the title mentions [IBD]?
>
> I think block serialization being fast could be useful for future kernel users. Reading and de-serializing blocks is one of the common operations I'd expect outside of just pure block validation.
Ok, seems fine. It would be good to list the main motivation(s) in the OP. Currently, it only says IBD and that IBD *isn't* the main motivation. However, I don't understand how this
...
💬 l0rinc commented on pull request "log: print reason when writing chainstate":
(https://github.com/bitcoin/bitcoin/pull/32404#discussion_r2072622314)
Is this only needed when we're using different durations for the parameters such as `ctx.randrange<std::chrono::seconds>(2s, 2min)`?
The result is a bit ugly now, but I've changed it.
(https://github.com/bitcoin/bitcoin/pull/32404#discussion_r2072622314)
Is this only needed when we're using different durations for the parameters such as `ctx.randrange<std::chrono::seconds>(2s, 2min)`?
The result is a bit ugly now, but I've changed it.
💬 l0rinc commented on pull request "log: print reason when writing chainstate":
(https://github.com/bitcoin/bitcoin/pull/32404#discussion_r2072622329)
I'm still not sure why we're adding them, but made sure the IDE adds newlines to the end now.
(https://github.com/bitcoin/bitcoin/pull/32404#discussion_r2072622329)
I'm still not sure why we're adding them, but made sure the IDE adds newlines to the end now.
💬 l0rinc commented on pull request "[IBD] specialize block serialization":
(https://github.com/bitcoin/bitcoin/pull/31868#issuecomment-2849231289)
I will work on this more in the upcoming weeks, drafted it for now since I don't like the current single-byte specializations, have to find a better way. Suggestions are welcome.
(https://github.com/bitcoin/bitcoin/pull/31868#issuecomment-2849231289)
I will work on this more in the upcoming weeks, drafted it for now since I don't like the current single-byte specializations, have to find a better way. Suggestions are welcome.
📝 andrewtoth reopened a pull request: "validation: periodically flush dbcache during reindex-chainstate"
(https://github.com/bitcoin/bitcoin/pull/32414)
After #30611 we periodically do a non-erasing flush of the dbcache to disk roughly every hour during IBD.
The intention was to also do this periodic flush during reindex-chainstate, so we would not risk losing progress during a system failure when reindexing with a high dbcache value.
It was discovered that reindex-chainstate does not perform a PERIODIC flush until it has already reached the tip. Since reindexing to tip usually happens within 24 hours, this behaviour was unnoticed with the p
...
(https://github.com/bitcoin/bitcoin/pull/32414)
After #30611 we periodically do a non-erasing flush of the dbcache to disk roughly every hour during IBD.
The intention was to also do this periodic flush during reindex-chainstate, so we would not risk losing progress during a system failure when reindexing with a high dbcache value.
It was discovered that reindex-chainstate does not perform a PERIODIC flush until it has already reached the tip. Since reindexing to tip usually happens within 24 hours, this behaviour was unnoticed with the p
...
💬 maflcko commented on pull request "[IBD] specialize block serialization":
(https://github.com/bitcoin/bitcoin/pull/31868#issuecomment-2849258766)
My guess is that the concept specialization here at most could have an effect on xor'd IO? This is because anything not xor'd won't need a buffer to (un)do the xor before reading or writing.
Thus, it just seems easier to use the existing BufferedReader/Writer in places that could benefit from it?
However, for block serialization they are already used.
Again, no objection here, but it would be good to explain:
* What is the goal of the changes here, and what real-world usage do they a
...
(https://github.com/bitcoin/bitcoin/pull/31868#issuecomment-2849258766)
My guess is that the concept specialization here at most could have an effect on xor'd IO? This is because anything not xor'd won't need a buffer to (un)do the xor before reading or writing.
Thus, it just seems easier to use the existing BufferedReader/Writer in places that could benefit from it?
However, for block serialization they are already used.
Again, no objection here, but it would be good to explain:
* What is the goal of the changes here, and what real-world usage do they a
...
💬 maflcko commented on pull request "log: print reason when writing chainstate":
(https://github.com/bitcoin/bitcoin/pull/32404#discussion_r2072639000)
> I'm still not sure why we're adding them, but made sure the IDE adds newlines to the end now.
* They are already present, so removing them is just pointless churn
* It is documented in `.editorconfig` and most editors add them by default, if missing. Which will cause churn in the future if they are removed on some files.
* This repo is using git as content management system, which complains about this. Also GitHub web and GitHub diff warn about this. E.g. see the tail of https://github.co
...
(https://github.com/bitcoin/bitcoin/pull/32404#discussion_r2072639000)
> I'm still not sure why we're adding them, but made sure the IDE adds newlines to the end now.
* They are already present, so removing them is just pointless churn
* It is documented in `.editorconfig` and most editors add them by default, if missing. Which will cause churn in the future if they are removed on some files.
* This repo is using git as content management system, which complains about this. Also GitHub web and GitHub diff warn about this. E.g. see the tail of https://github.co
...
💬 maflcko commented on pull request "log: print reason when writing chainstate":
(https://github.com/bitcoin/bitcoin/pull/32404#discussion_r2072639006)
> Is this only needed when we're using different durations for the parameters such as
No. The function above only takes one parameter. The reason is explained in the comment of the function immediately above. (Line 338)
(https://github.com/bitcoin/bitcoin/pull/32404#discussion_r2072639006)
> Is this only needed when we're using different durations for the parameters such as
No. The function above only takes one parameter. The reason is explained in the comment of the function immediately above. (Line 338)
🤔 furszy reviewed a pull request: "crypto: Use secure_allocator for `AES256_ctx`"
(https://github.com/bitcoin/bitcoin/pull/31774#pullrequestreview-2813492635)
I don't see why the first commit is necessary. Couldn't you just mock the time so that it's fixed and the number of derivation rounds always stays at the default?
(https://github.com/bitcoin/bitcoin/pull/31774#pullrequestreview-2813492635)
I don't see why the first commit is necessary. Couldn't you just mock the time so that it's fixed and the number of derivation rounds always stays at the default?
💬 hebasto commented on pull request "test: Suppress Windows abort message in CI":
(https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-2849289055)
@maflcko
> No objection, but assert/Assert seems to be used widely in the codebase, so shouldn't this be done for all test binaries, or none? Otherwise the same assert could lead to inconsistent behavior, depending on which test binary ran into it.
Thanks! Reworked.
(https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-2849289055)
@maflcko
> No objection, but assert/Assert seems to be used widely in the codebase, so shouldn't this be done for all test binaries, or none? Otherwise the same assert could lead to inconsistent behavior, depending on which test binary ran into it.
Thanks! Reworked.
💬 l0rinc commented on pull request "log: print reason when writing chainstate":
(https://github.com/bitcoin/bitcoin/pull/32404#discussion_r2072654348)
Yeah, I know tgese, but it still doesn't explain why - metaphorically speaking - we need a comma after the last element in a list. But I've configured my IDE now to add it, I don't have to agree to accept it :)
(https://github.com/bitcoin/bitcoin/pull/32404#discussion_r2072654348)
Yeah, I know tgese, but it still doesn't explain why - metaphorically speaking - we need a comma after the last element in a list. But I've configured my IDE now to add it, I don't have to agree to accept it :)
💬 maflcko commented on pull request "log: print reason when writing chainstate":
(https://github.com/bitcoin/bitcoin/pull/32404#discussion_r2072657473)
> Yeah, I know these, but it still doesn't explain why - metaphorically speaking - we need a comma after the last element in a list.
The reason is the same: If there weren't a comma (newline) after the last element (line) in a list (file), a diff would be verbose when a new element (line) is added after the last. Also, the verbose diff breaks `git blame`. Finally, tools may misbehave when the tool or the user assumes a newline is present.
(https://github.com/bitcoin/bitcoin/pull/32404#discussion_r2072657473)
> Yeah, I know these, but it still doesn't explain why - metaphorically speaking - we need a comma after the last element in a list.
The reason is the same: If there weren't a comma (newline) after the last element (line) in a list (file), a diff would be verbose when a new element (line) is added after the last. Also, the verbose diff breaks `git blame`. Finally, tools may misbehave when the tool or the user assumes a newline is present.