š¬ brunoerg commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2081903903)
`ConsumeTransaction` is fine.
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2081903903)
`ConsumeTransaction` is fine.
š¬ hebasto commented on pull request "cmake: Introduce `WITH_PYTHON` build option":
(https://github.com/bitcoin/bitcoin/pull/31669#issuecomment-2866948636)
The [feedback](https://github.com/bitcoin/bitcoin/pull/31669#discussion_r2081903398) from @maflcko has been addressed.
Also rebased.
(https://github.com/bitcoin/bitcoin/pull/31669#issuecomment-2866948636)
The [feedback](https://github.com/bitcoin/bitcoin/pull/31669#discussion_r2081903398) from @maflcko has been addressed.
Also rebased.
š¤ fanquake requested changes to a pull request: "cmake: Introduce `WITH_PYTHON` build option"
(https://github.com/bitcoin/bitcoin/pull/31669#pullrequestreview-2828742547)
As I mentioned above, adding more build options/compexity, and forcing builders to opt out of things they don't care about, does not seem like a good, nor generic approach to fixing #31476. Especailly given this is essentially a CI fix, to work around the fact that CMake doesn't have a way to turn warnings into errors?
(https://github.com/bitcoin/bitcoin/pull/31669#pullrequestreview-2828742547)
As I mentioned above, adding more build options/compexity, and forcing builders to opt out of things they don't care about, does not seem like a good, nor generic approach to fixing #31476. Especailly given this is essentially a CI fix, to work around the fact that CMake doesn't have a way to turn warnings into errors?
š¬ fanquake commented on pull request "cmake: Introduce `WITH_PYTHON` build option":
(https://github.com/bitcoin/bitcoin/pull/31669#discussion_r2081908186)
> That concern still appears to be relevant.
Yea. This is also essentially reintroducing the previous Autotools behaviour of having opt-out dependencies (i.e bdb for a long time), rather than opt-in, which is what we started doing with CMake.
(https://github.com/bitcoin/bitcoin/pull/31669#discussion_r2081908186)
> That concern still appears to be relevant.
Yea. This is also essentially reintroducing the previous Autotools behaviour of having opt-out dependencies (i.e bdb for a long time), rather than opt-in, which is what we started doing with CMake.
š¬ laanwj commented on pull request "bench: replace benchmark block with more representative one (413567 ā 784588)":
(https://github.com/bitcoin/bitcoin/pull/32457#discussion_r2081925616)
it's not just the amount of data, we're still scared from the xz backdoor incident :smile:
(https://github.com/bitcoin/bitcoin/pull/32457#discussion_r2081925616)
it's not just the amount of data, we're still scared from the xz backdoor incident :smile:
š¬ maflcko commented on pull request "cmake: Introduce `WITH_PYTHON` build option":
(https://github.com/bitcoin/bitcoin/pull/31669#issuecomment-2866998704)
> forcing builders to opt out of things they don't care about
I am not sure the current approach of silently skipping tests (with just a warning during configure, not when running the tests) is the right approach.
A third alternative would be to delay the error to when they run the tests.
A fourth alternative would be to just call the two python tests from the functional test runner somehow and remove all this code here?
(https://github.com/bitcoin/bitcoin/pull/31669#issuecomment-2866998704)
> forcing builders to opt out of things they don't care about
I am not sure the current approach of silently skipping tests (with just a warning during configure, not when running the tests) is the right approach.
A third alternative would be to delay the error to when they run the tests.
A fourth alternative would be to just call the two python tests from the functional test runner somehow and remove all this code here?
š¬ hebasto commented on pull request "cmake: Introduce `WITH_PYTHON` build option":
(https://github.com/bitcoin/bitcoin/pull/31669#issuecomment-2867005648)
> > forcing builders to opt out of things they don't care about
>
> I am not sure the current approach of silently skipping tests (with just a warning during configure, not when running the tests) is the right approach.
>
> A third alternative would be to delay the error to when they run the tests.
FWIW, something similar has been implemented in https://github.com/bitcoin/bitcoin/pull/31233.
(https://github.com/bitcoin/bitcoin/pull/31669#issuecomment-2867005648)
> > forcing builders to opt out of things they don't care about
>
> I am not sure the current approach of silently skipping tests (with just a warning during configure, not when running the tests) is the right approach.
>
> A third alternative would be to delay the error to when they run the tests.
FWIW, something similar has been implemented in https://github.com/bitcoin/bitcoin/pull/31233.
š¬ fanquake commented on pull request "test: Suppress Windows abort message in CI":
(https://github.com/bitcoin/bitcoin/pull/32409#discussion_r2081949565)
I think the commit message should still be fixed to say `When compiling with MSVC`, rather than `On Windows`?
> (I've added a note to the PR description)
Is it a bug in mingw-w64 that it doesn't match the MSVC behaviour? It would be good to an explanation of the difference in the PR description/commit message, as it isn't really clear from going to the link you've provided.
(https://github.com/bitcoin/bitcoin/pull/32409#discussion_r2081949565)
I think the commit message should still be fixed to say `When compiling with MSVC`, rather than `On Windows`?
> (I've added a note to the PR description)
Is it a bug in mingw-w64 that it doesn't match the MSVC behaviour? It would be good to an explanation of the difference in the PR description/commit message, as it isn't really clear from going to the link you've provided.
š¬ fanquake commented on pull request "test: Suppress Windows abort message in CI":
(https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-2867023954)
> It is documented, and it is pre-defined by CI itself:
It's not in our docs, or in our CI code, which means our CI would "work", because a third party is putting something into the environment, and the fact that this is even happening, is only discoverable if you happen to read a `.cpp` file.
Would someone running these binaries locally also need/want to set this env var to get the same behaviour? If so, how would they figure that out?
(https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-2867023954)
> It is documented, and it is pre-defined by CI itself:
It's not in our docs, or in our CI code, which means our CI would "work", because a third party is putting something into the environment, and the fact that this is even happening, is only discoverable if you happen to read a `.cpp` file.
Would someone running these binaries locally also need/want to set this env var to get the same behaviour? If so, how would they figure that out?
š¬ hebasto commented on pull request "test: Suppress Windows abort message in CI":
(https://github.com/bitcoin/bitcoin/pull/32409#discussion_r2081956577)
> Is it a bug in mingw-w64 that it doesn't match the MSVC behaviour?
I don't think so, given the different runtime libraries used. I must admit I haven't tested cross-compiled binaries linked to UCRT.
(https://github.com/bitcoin/bitcoin/pull/32409#discussion_r2081956577)
> Is it a bug in mingw-w64 that it doesn't match the MSVC behaviour?
I don't think so, given the different runtime libraries used. I must admit I haven't tested cross-compiled binaries linked to UCRT.
ā ļø ismaelsadeeq opened an issue: "RFC: Should node Wallet Startup Options Apply to Individual Wallets?"
(https://github.com/bitcoin/bitcoin/issues/32462)
PR #29278 is going to introduces a `maxfeerate` wallet startup option.
There was some discussion between between @luke-jr , me, and @murchandamus in a thread https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1468533274
> This still isn't a wallet option...
> I don't understand why you said it is not a wallet option.
> The `OptionsCategory` of this startup option is `WALLET`, and it's only used in the wallet.
> Can you please expand on your comment?
> Thanks.
> Iām also confused by t
...
(https://github.com/bitcoin/bitcoin/issues/32462)
PR #29278 is going to introduces a `maxfeerate` wallet startup option.
There was some discussion between between @luke-jr , me, and @murchandamus in a thread https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1468533274
> This still isn't a wallet option...
> I don't understand why you said it is not a wallet option.
> The `OptionsCategory` of this startup option is `WALLET`, and it's only used in the wallet.
> Can you please expand on your comment?
> Thanks.
> Iām also confused by t
...
š¬ ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2081959909)
I opened a separate issue for this here https://github.com/bitcoin/bitcoin/issues/32462#issue-3052551984
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r2081959909)
I opened a separate issue for this here https://github.com/bitcoin/bitcoin/issues/32462#issue-3052551984
š¬ l0rinc commented on pull request "bench: replace benchmark block with more representative one (413567 ā 784588)":
(https://github.com/bitcoin/bitcoin/pull/32457#discussion_r2081962158)
Understandable, but that's why I added the hashes here, to make it self-validating.
(https://github.com/bitcoin/bitcoin/pull/32457#discussion_r2081962158)
Understandable, but that's why I added the hashes here, to make it self-validating.
š fanquake merged a pull request: "refactor: Removals after bdb removal"
(https://github.com/bitcoin/bitcoin/pull/32438)
(https://github.com/bitcoin/bitcoin/pull/32438)
š¬ hebasto commented on pull request "test: Suppress Windows abort message in CI":
(https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-2867062653)
> > It is documented, and it is pre-defined by CI itself:
>
> It's not in our docs, or in our CI code, which means our CI would "work", because a third party is putting something into the environment, and the fact that this is even happening, is only discoverable if you happen to read a `.cpp` file.
What are you suggesting should be changed to improve or avoid this situation?
> Would someone running these binaries locally also need/want to set this env var to get the same behaviour? If
...
(https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-2867062653)
> > It is documented, and it is pre-defined by CI itself:
>
> It's not in our docs, or in our CI code, which means our CI would "work", because a third party is putting something into the environment, and the fact that this is even happening, is only discoverable if you happen to read a `.cpp` file.
What are you suggesting should be changed to improve or avoid this situation?
> Would someone running these binaries locally also need/want to set this env var to get the same behaviour? If
...
š¬ fanquake commented on pull request "test: Suppress Windows abort message in CI":
(https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-2867064817)
> This does not apply to the "raw" GitHub Actions workflows, where this code takes effect.
Can't someone just compile and run the same binaries locally?
(https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-2867064817)
> This does not apply to the "raw" GitHub Actions workflows, where this code takes effect.
Can't someone just compile and run the same binaries locally?
š¬ hebasto commented on pull request "test: Suppress Windows abort message in CI":
(https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-2867073689)
> > This does not apply to the "raw" GitHub Actions workflows, where this code takes effect.
>
> Can't someone just compile and run the same binaries locally?
Certainly, they can. But displaying a message box isn't an issue when running binaries locally, so there's no need to alter the environment in that case.
(https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-2867073689)
> > This does not apply to the "raw" GitHub Actions workflows, where this code takes effect.
>
> Can't someone just compile and run the same binaries locally?
Certainly, they can. But displaying a message box isn't an issue when running binaries locally, so there's no need to alter the environment in that case.
š¤ pablomartin4btc reviewed a pull request: "wallet: init, don't error out when loading legacy wallets"
(https://github.com/bitcoin/bitcoin/pull/32449#pullrequestreview-2828866727)
Concept ACK
You could have a few/ several legacy wallets on the `settings.json`. Perhaps list them all together on a warning or just warn that there is at least one (on the GUI you would get a pop up on each otherwise).
(https://github.com/bitcoin/bitcoin/pull/32449#pullrequestreview-2828866727)
Concept ACK
You could have a few/ several legacy wallets on the `settings.json`. Perhaps list them all together on a warning or just warn that there is at least one (on the GUI you would get a pop up on each otherwise).
š¬ pablomartin4btc commented on pull request "wallet: init, don't error out when loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/32449#discussion_r2081981059)
_<ins>nit</ins>_: it's not a failure on a load, it's not allowed/ possible anymore... just a suggestion, could be another text message... (also checking a flag to send the warning only once... or with the list of all of them if many)
```suggestion
warnings.push_back(strprintf(_("There are legacy wallets in settings.json which are no longer supported.\n\nPlease migrate to a descriptor wallet using the migration tool (migratewallet RPC or the GUI option)."), walletFile));
```
(https://github.com/bitcoin/bitcoin/pull/32449#discussion_r2081981059)
_<ins>nit</ins>_: it's not a failure on a load, it's not allowed/ possible anymore... just a suggestion, could be another text message... (also checking a flag to send the warning only once... or with the list of all of them if many)
```suggestion
warnings.push_back(strprintf(_("There are legacy wallets in settings.json which are no longer supported.\n\nPlease migrate to a descriptor wallet using the migration tool (migratewallet RPC or the GUI option)."), walletFile));
```
š¬ hebasto commented on pull request "test: Suppress Windows abort message in CI":
(https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-2867088113)
As an alternative, we could explicitly set our own `SUPPRESS_ABORT_MESSAGE_BOX` environment variable in the workflow, and gate the code with this variable instead, removing the `#if defined(_MSC_VER)` condition.
(https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-2867088113)
As an alternative, we could explicitly set our own `SUPPRESS_ABORT_MESSAGE_BOX` environment variable in the workflow, and gate the code with this variable instead, removing the `#if defined(_MSC_VER)` condition.