💬 mzumsande commented on pull request "fuzz: provide more realistic values to the base58(check) decoders":
(https://github.com/bitcoin/bitcoin/pull/31917#discussion_r1994137293)
ok, makes sense, but I'd challenge that this is "a bug that could cause a fuzz failure" (otherwise I would've expected this failure to be found by now):
My understanding is that in the relevant special case, both `decoded` an `encoded_string` are empty. However, it should be impossible for `encoded_string` to be empty and `decoded` non-empty (and vice versa) because of how base58 works.
(https://github.com/bitcoin/bitcoin/pull/31917#discussion_r1994137293)
ok, makes sense, but I'd challenge that this is "a bug that could cause a fuzz failure" (otherwise I would've expected this failure to be found by now):
My understanding is that in the relevant special case, both `decoded` an `encoded_string` are empty. However, it should be impossible for `encoded_string` to be empty and `decoded` non-empty (and vice versa) because of how base58 works.
💬 mzumsande commented on pull request "fuzz: provide more realistic values to the base58(check) decoders":
(https://github.com/bitcoin/bitcoin/pull/31917#issuecomment-2722394679)
Code Review / lightly tested ACK d5537c18a9034647ba4c9ed4008abd7fee33989e
(https://github.com/bitcoin/bitcoin/pull/31917#issuecomment-2722394679)
Code Review / lightly tested ACK d5537c18a9034647ba4c9ed4008abd7fee33989e
💬 furszy commented on issue "Wallet migration includes rescan":
(https://github.com/bitcoin/bitcoin/issues/32045#issuecomment-2722396445)
> Ideally the migration should finish and then prompt the user that a rescan is about to happen. That way the user can abort the rescan and resume it some other time if it takes too long.
Sadly, we cannot do that right away. The rescan is being triggered by the loading procedure when the wallet is reloaded from disk after migration. We could add a "no scan during init" option (this should also be a new init flag).
> Alternatively, the migrate wallet dialog could at least warn that a rescan is
...
(https://github.com/bitcoin/bitcoin/issues/32045#issuecomment-2722396445)
> Ideally the migration should finish and then prompt the user that a rescan is about to happen. That way the user can abort the rescan and resume it some other time if it takes too long.
Sadly, we cannot do that right away. The rescan is being triggered by the loading procedure when the wallet is reloaded from disk after migration. We could add a "no scan during init" option (this should also be a new init flag).
> Alternatively, the migrate wallet dialog could at least warn that a rescan is
...
🚀 ryanofsky merged a pull request: "wallet: fix crash on double block disconnection"
(https://github.com/bitcoin/bitcoin/pull/31757)
(https://github.com/bitcoin/bitcoin/pull/31757)
💬 hodlinator commented on pull request "qa: Fix TxIndex race conditions":
(https://github.com/bitcoin/bitcoin/pull/32010#issuecomment-2722483103)
> Are the files mentioned in `files_to_delete` and `files_to_perturb` comments just for informational purposes that they don't trigger an error or you want to indicate that these files should trigger an error but currently they don't.
Yes, some of them at least probably should trigger an error, but I'm not knowledgeable of those aspects of LevelDB. It would probably be good to dig deeper into that, but I wanted to keep the focus of this PR on the tx index race conditions.
The reason I touc
...
(https://github.com/bitcoin/bitcoin/pull/32010#issuecomment-2722483103)
> Are the files mentioned in `files_to_delete` and `files_to_perturb` comments just for informational purposes that they don't trigger an error or you want to indicate that these files should trigger an error but currently they don't.
Yes, some of them at least probably should trigger an error, but I'm not knowledgeable of those aspects of LevelDB. It would probably be good to dig deeper into that, but I wanted to keep the focus of this PR on the tx index race conditions.
The reason I touc
...
📝 pinheadmz opened a pull request: "[draft] Replace libevent with our own HTTP and socket-handling implementation"
(https://github.com/bitcoin/bitcoin/pull/32061)
This is a major component of [removing libevent as a dependency of the project](https://github.com/bitcoin/bitcoin/issues/31194)
It is based on #30988 but only in the sense that it consumes the `Sockman` class introduced in that PR. The p2p / `Connman` rebase isn't needed for HTTP and therefore this branch could be refactored to only include `sockman.{h,cpp}`.
Commit strategy:
- Assert current behavior of HTTP with additional functional tests, including copying from libevent's tests
- I
...
(https://github.com/bitcoin/bitcoin/pull/32061)
This is a major component of [removing libevent as a dependency of the project](https://github.com/bitcoin/bitcoin/issues/31194)
It is based on #30988 but only in the sense that it consumes the `Sockman` class introduced in that PR. The p2p / `Connman` rebase isn't needed for HTTP and therefore this branch could be refactored to only include `sockman.{h,cpp}`.
Commit strategy:
- Assert current behavior of HTTP with additional functional tests, including copying from libevent's tests
- I
...
💬 pinheadmz commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2722497291)
I just opened https://github.com/bitcoin/bitcoin/pull/32061 which consumes `Sockman` for HTTP. Thanks to @theuni help at coredev I'm now pretty happy with the performance. It obviously doesn't require a p2p refactor -- so we could just review sockman on its own, or in the context of HTTP
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2722497291)
I just opened https://github.com/bitcoin/bitcoin/pull/32061 which consumes `Sockman` for HTTP. Thanks to @theuni help at coredev I'm now pretty happy with the performance. It obviously doesn't require a p2p refactor -- so we could just review sockman on its own, or in the context of HTTP
📝 pinheadmz converted_to_draft a pull request: "[draft] Replace libevent with our own HTTP and socket-handling implementation"
(https://github.com/bitcoin/bitcoin/pull/32061)
This is a major component of [removing libevent as a dependency of the project](https://github.com/bitcoin/bitcoin/issues/31194)
It is based on #30988 but only in the sense that it consumes the `Sockman` class introduced in that PR. The p2p / `Connman` rebase isn't needed for HTTP and therefore this branch could be refactored to only include `sockman.{h,cpp}`.
Commit strategy:
- Assert current behavior of HTTP with additional functional tests, including copying from libevent's tests
- I
...
(https://github.com/bitcoin/bitcoin/pull/32061)
This is a major component of [removing libevent as a dependency of the project](https://github.com/bitcoin/bitcoin/issues/31194)
It is based on #30988 but only in the sense that it consumes the `Sockman` class introduced in that PR. The p2p / `Connman` rebase isn't needed for HTTP and therefore this branch could be refactored to only include `sockman.{h,cpp}`.
Commit strategy:
- Assert current behavior of HTTP with additional functional tests, including copying from libevent's tests
- I
...
💬 brunoerg commented on pull request "contrib: Update coverage.cpp macro to support macOS":
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1994209990)
I think linux is more permissive than macOS that's why CI is failing. The jobs do not set `-fprofile-instr-generate -fcoverage-mapping`. I did not test, but using lld could work?
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1994209990)
I think linux is more permissive than macOS that's why CI is failing. The jobs do not set `-fprofile-instr-generate -fcoverage-mapping`. I did not test, but using lld could work?
👍 hodlinator approved a pull request: "ci: Test cross-built Windows executables on Windows natively"
(https://github.com/bitcoin/bitcoin/pull/31176#pullrequestreview-2683225882)
re-ACK 565184e6e81a58c9cf60081cdced9a8d47725664
range-diff with [previous ACK](https://github.com/bitcoin/bitcoin/pull/31176#pullrequestreview-2661348106) confirmed only binary path fixups stemming from #31161 - happy that finally landed! :tada:
(https://github.com/bitcoin/bitcoin/pull/31176#pullrequestreview-2683225882)
re-ACK 565184e6e81a58c9cf60081cdced9a8d47725664
range-diff with [previous ACK](https://github.com/bitcoin/bitcoin/pull/31176#pullrequestreview-2661348106) confirmed only binary path fixups stemming from #31161 - happy that finally landed! :tada:
👍 ryanofsky approved a pull request: "qa: Verify clean shutdown on startup failure"
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2683088562)
Code review ACK c107aa8c873cb3c031b341baef487638133823f1. Changes since last review were just updating some messages and expanding the test a little. Thanks for the updates. This still looks good to me and I left some more comments, but they are not important.
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2683088562)
Code review ACK c107aa8c873cb3c031b341baef487638133823f1. Changes since last review were just updating some messages and expanding the test a little. Thanks for the updates. This still looks good to me and I left some more comments, but they are not important.
💬 ryanofsky commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1994143659)
re: https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1954056589
> Thanks! This made me actually realize I could add a test for it and improve the exception message (+ commit message).
I'm still not clear when this case would ever be expected to happen.
IIUC, there is no runtime error that could trigger this, and nothing non-test code could could do to trigger it, and the only thing that could trigger it is incorrectly written test code?
If so, it would seem clearer to replac
...
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1994143659)
re: https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1954056589
> Thanks! This made me actually realize I could add a test for it and improve the exception message (+ commit message).
I'm still not clear when this case would ever be expected to happen.
IIUC, there is no runtime error that could trigger this, and nothing non-test code could could do to trigger it, and the only thing that could trigger it is incorrectly written test code?
If so, it would seem clearer to replac
...
💬 ryanofsky commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1994186307)
re: https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1953499371
> I think it is good to document _why_ we are catching and suppressing some exceptions (while re-raising others).
It's funny, because to me replacing "Initialization phase" with "Suppress these in favor of a later outcome" is literally *removing* the comment that explains why it is ok to suppress these errors. The previous comment made it clear these errors were being suppressed because they are expected during initia
...
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1994186307)
re: https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1953499371
> I think it is good to document _why_ we are catching and suppressing some exceptions (while re-raising others).
It's funny, because to me replacing "Initialization phase" with "Suppress these in favor of a later outcome" is literally *removing* the comment that explains why it is ok to suppress these errors. The previous comment made it clear these errors were being suppressed because they are expected during initia
...
💬 jonatack commented on pull request "[IBD] flush UXTOs in bigger batches":
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2722607551)
Review and testing-under-way ACK 868413340f8d6058d74186b65ac3498d6b7f254a
As described in the pull description, this value was set in 2017 in #10148 and hasn't been changed since.
> Maybe a config option can be provided to change it.
> Just realized [`dbbatchsize`](https://github.com/bitcoin/bitcoin/blob/35bf426e02210c1bbb04926f4ca2e0285fbfcd11/src/init.cpp#L489) already exists.
This is a hidden config option and the rationale might be https://github.com/bitcoin/bitcoin/pull/10148#di
...
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2722607551)
Review and testing-under-way ACK 868413340f8d6058d74186b65ac3498d6b7f254a
As described in the pull description, this value was set in 2017 in #10148 and hasn't been changed since.
> Maybe a config option can be provided to change it.
> Just realized [`dbbatchsize`](https://github.com/bitcoin/bitcoin/blob/35bf426e02210c1bbb04926f4ca2e0285fbfcd11/src/init.cpp#L489) already exists.
This is a hidden config option and the rationale might be https://github.com/bitcoin/bitcoin/pull/10148#di
...
🚀 ryanofsky merged a pull request: "qa: clarify and document one assumeutxo test case with malleated snapshot"
(https://github.com/bitcoin/bitcoin/pull/31907)
(https://github.com/bitcoin/bitcoin/pull/31907)
💬 ryanofsky commented on pull request "qa: clarify and document one assumeutxo test case with malleated snapshot":
(https://github.com/bitcoin/bitcoin/pull/31907#issuecomment-2722632443)
Merged since it was requested in IRC. Could be useful to follow up on comments from https://github.com/bitcoin/bitcoin/pull/31907#pullrequestreview-2661853159 though
(https://github.com/bitcoin/bitcoin/pull/31907#issuecomment-2722632443)
Merged since it was requested in IRC. Could be useful to follow up on comments from https://github.com/bitcoin/bitcoin/pull/31907#pullrequestreview-2661853159 though
💬 TheCharlatan commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#issuecomment-2722641649)
> I think if multiple chainstates will exist for different purposes, the chainstate objects will need to have some field describing their purpose and controlling their behavior.
Yes, I think I have come to a similar conclusion, and I think the introduced reliance here on the `m_target_blockhash` and `m_from_snapshot_blockhash` are very good changes. Having some kind of "start" and "stop" blocks for a chainstate seems like something that is easy to understand and might also be useful beyond a
...
(https://github.com/bitcoin/bitcoin/pull/30214#issuecomment-2722641649)
> I think if multiple chainstates will exist for different purposes, the chainstate objects will need to have some field describing their purpose and controlling their behavior.
Yes, I think I have come to a similar conclusion, and I think the introduced reliance here on the `m_target_blockhash` and `m_from_snapshot_blockhash` are very good changes. Having some kind of "start" and "stop" blocks for a chainstate seems like something that is easy to understand and might also be useful beyond a
...
💬 TheCharlatan commented on pull request "qa: clarify and document one assumeutxo test case with malleated snapshot":
(https://github.com/bitcoin/bitcoin/pull/31907#issuecomment-2722677016)
Post-merge ACK
(https://github.com/bitcoin/bitcoin/pull/31907#issuecomment-2722677016)
Post-merge ACK
🚀 glozow merged a pull request: "contrib: Fix `gen-bitcoin-conf.sh`"
(https://github.com/bitcoin/bitcoin/pull/32049)
(https://github.com/bitcoin/bitcoin/pull/32049)
🤔 furszy reviewed a pull request: "qa: Fix TxIndex race conditions"
(https://github.com/bitcoin/bitcoin/pull/32010#pullrequestreview-2683350728)
Code review ACK 3301d2cbe8c3b76c97285d75fa59637cb6952d0b
(https://github.com/bitcoin/bitcoin/pull/32010#pullrequestreview-2683350728)
Code review ACK 3301d2cbe8c3b76c97285d75fa59637cb6952d0b