💬 ajtowns commented on issue "Slow unit tests delay functional tests and leave CPU unused":
(https://github.com/bitcoin/bitcoin/issues/32770#issuecomment-3081928184)
cf #32945 maybe?
Changing test_runner.py from python to cmake logic might make running the functional and unit tests in parallel more plausible?
(https://github.com/bitcoin/bitcoin/issues/32770#issuecomment-3081928184)
cf #32945 maybe?
Changing test_runner.py from python to cmake logic might make running the functional and unit tests in parallel more plausible?
💬 davidgumberg commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2211907184)
Thank you, fixed!
(https://github.com/bitcoin/bitcoin/pull/32273#discussion_r2211907184)
Thank you, fixed!
💬 davidgumberg commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-3081990855)
Rebased to address @pablomartin4btc and merge conflict.
(https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-3081990855)
Rebased to address @pablomartin4btc and merge conflict.
💬 ajtowns commented on pull request "Bump SCRIPT_VERIFY flags to 64 bit":
(https://github.com/bitcoin/bitcoin/pull/32998#issuecomment-3082011233)
CI failure seems to be due to [a bug in qt6 6.4](https://bugreports.qt.io/browse/QTBUG-31496?focusedId=888930&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-888930):
> Also for people coming here from Google: I had the same error message, but for me the problem was that I was using Qt 6.4.2 which apparently thinks that #if 202002L < 201103L is true, which causes c++config.h to not be included (and no #error is generated because moc doesn't support the directiv
...
(https://github.com/bitcoin/bitcoin/pull/32998#issuecomment-3082011233)
CI failure seems to be due to [a bug in qt6 6.4](https://bugreports.qt.io/browse/QTBUG-31496?focusedId=888930&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-888930):
> Also for people coming here from Google: I had the same error message, but for me the problem was that I was using Qt 6.4.2 which apparently thinks that #if 202002L < 201103L is true, which causes c++config.h to not be included (and no #error is generated because moc doesn't support the directiv
...
💬 ajtowns commented on pull request "tests: speed up coins_tests by parallelizing":
(https://github.com/bitcoin/bitcoin/pull/32945#issuecomment-3082477608)
> Given that the `and`s and `also`s in the description, I personally would find it useful to split the PR into smaller commits accordingly. I'd ACK the `coins_tests.cpp` changes (had problems because of that simulation test being part of the suite numerous times), no opinion about the rest.
Without the lint changes, having multiple test fixtures in a single cpp file causes CI failures. Without the cmake changes, the additional test fixtures aren't executed via ctest. So I don't think it makes
...
(https://github.com/bitcoin/bitcoin/pull/32945#issuecomment-3082477608)
> Given that the `and`s and `also`s in the description, I personally would find it useful to split the PR into smaller commits accordingly. I'd ACK the `coins_tests.cpp` changes (had problems because of that simulation test being part of the suite numerous times), no opinion about the rest.
Without the lint changes, having multiple test fixtures in a single cpp file causes CI failures. Without the cmake changes, the additional test fixtures aren't executed via ctest. So I don't think it makes
...
🤔 l0rinc requested changes to a pull request: "Enhanced error messages for invalid network prefix during address parsing."
(https://github.com/bitcoin/bitcoin/pull/27260#pullrequestreview-3027286087)
Concept ACK, we should indeed make the error messages more user friendly.
It seems to me the PR is heading in the right direction, but we need to separate handling unrelated code paths - bech32 and base58 are different enough that they deserve their own helper method, not just buried somewhere in an if condition.
The functional tests could also use better separation, without favoring one or the other in the default params and the commit messages should explain the context better.
(https://github.com/bitcoin/bitcoin/pull/27260#pullrequestreview-3027286087)
Concept ACK, we should indeed make the error messages more user friendly.
It seems to me the PR is heading in the right direction, but we need to separate handling unrelated code paths - bech32 and base58 are different enough that they deserve their own helper method, not just buried somewhere in an if condition.
The functional tests could also use better separation, without favoring one or the other in the default params and the commit messages should explain the context better.
💬 l0rinc commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2211854675)
Q: when do we refer to it as `Bech32` and when `Bech32(m)`?
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2211854675)
Q: when do we refer to it as `Bech32` and when `Bech32(m)`?
💬 l0rinc commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2211858218)
It would help with the review if commits only had a single focus: separating low risk refactors with higher risk behavioral changes.
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2211858218)
It would help with the review if commits only had a single focus: separating low risk refactors with higher risk behavioral changes.
💬 l0rinc commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2211859463)
what's the story behind these, could we add them to the test suite instead of deleting them?
Why were they committed like this in the first place?
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2211859463)
what's the story behind these, could we add them to the test suite instead of deleting them?
Why were they committed like this in the first place?
💬 l0rinc commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2211861199)
could we rather store them in a dictionary above, keyed by `network_name`?
```python
INVALID_DATA = {
"mainnet": [
("tc1qw508d6qejxtdg4y5r3zarvary0c5xw7kg3g4ty", "Invalid or unsupported prefix for Segwit (Bech32) Bitcoin address (expected bc, got tc)", []),
...
],
"signet": [
("tc1qw508d6qejxtdg4y5r3zarvary0c5xw7kg3g4ty", "Invalid or unsupported prefix for Segwit (Bech32) signet address (expected tb, got tc)", []),
...
],
}
VALID_DATA = {
"mainn
...
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2211861199)
could we rather store them in a dictionary above, keyed by `network_name`?
```python
INVALID_DATA = {
"mainnet": [
("tc1qw508d6qejxtdg4y5r3zarvary0c5xw7kg3g4ty", "Invalid or unsupported prefix for Segwit (Bech32) Bitcoin address (expected bc, got tc)", []),
...
],
"signet": [
("tc1qw508d6qejxtdg4y5r3zarvary0c5xw7kg3g4ty", "Invalid or unsupported prefix for Segwit (Bech32) signet address (expected tb, got tc)", []),
...
],
}
VALID_DATA = {
"mainn
...
💬 l0rinc commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2212232968)
We should find a way to avoid digging into the internals of `LocateErrors` - maybe we could return a proper type instead of `std::pair<std::string, std::vector<int>> LocateErrors` which would return the error category (as enum?) instead of the text directly - the users of `LocateErrors` should decide the exact error message per error code.
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2212232968)
We should find a way to avoid digging into the internals of `LocateErrors` - maybe we could return a proper type instead of `std::pair<std::string, std::vector<int>> LocateErrors` which would return the error category (as enum?) instead of the text directly - the users of `LocateErrors` should decide the exact error message per error code.
💬 l0rinc commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2212280989)
I find it confusing that when we don't have valid bech32 chars we report that it's not a valid base58
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2212280989)
I find it confusing that when we don't have valid bech32 chars we report that it's not a valid base58
💬 l0rinc commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2212237016)
we could invert these conditions to start with the non-negated versions, it flows more naturally to do `if (condition) ... else` compared to `if (!condition) ... else`
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2212237016)
we could invert these conditions to start with the non-negated versions, it flows more naturally to do `if (condition) ... else` compared to `if (!condition) ... else`
💬 l0rinc commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2212242754)
maybe we could just join them via comma instead, something like:
```C++
const auto prefixes{util::Join(encoded_prefixes, ", ")};
```
or combining it with the above:
```C++
const auto prefixes{util::Join(Cat(std::vector(params.Base58EncodedPrefix(CChainParams::SCRIPT_ADDRESS)),
params.Base58EncodedPrefix(CChainParams::PUBKEY_ADDRESS)), ", ")};
```
(untested and we'd likely have to adjust the full texts accordingly)
-----
or combining it with the
...
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2212242754)
maybe we could just join them via comma instead, something like:
```C++
const auto prefixes{util::Join(encoded_prefixes, ", ")};
```
or combining it with the above:
```C++
const auto prefixes{util::Join(Cat(std::vector(params.Base58EncodedPrefix(CChainParams::SCRIPT_ADDRESS)),
params.Base58EncodedPrefix(CChainParams::PUBKEY_ADDRESS)), ", ")};
```
(untested and we'd likely have to adjust the full texts accordingly)
-----
or combining it with the
...
💬 l0rinc commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2212239517)
I know this was here before as well, but we have already checked `is_bech32` in the if condition, we could break up `if (!is_bech32 && DecodeBase58Check(str, data, MAX_BASE58_CHECK_CHARS)) {` into an ` if (!is_bech32) { if (DecodeBase58Check) ... } else { ... }` instead - preferably inverting it as well to `if (is_bech32) {} else if (DecodeBase58Check...)`. If you decide to accept these comments, please do it in multiple, focused commits, the changes should guide the reviewer and tell a story.
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r2212239517)
I know this was here before as well, but we have already checked `is_bech32` in the if condition, we could break up `if (!is_bech32 && DecodeBase58Check(str, data, MAX_BASE58_CHECK_CHARS)) {` into an ` if (!is_bech32) { if (DecodeBase58Check) ... } else { ... }` instead - preferably inverting it as well to `if (is_bech32) {} else if (DecodeBase58Check...)`. If you decide to accept these comments, please do it in multiple, focused commits, the changes should guide the reviewer and tell a story.
💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2212388363)
> Actually I feel a bit less convinced that dropping a `sudo ... ` command into the CI scripts is ideal
Yeah, they should be fine to run as a user normally. The only dependencies are Python, Bash, and a container engine.
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2212388363)
> Actually I feel a bit less convinced that dropping a `sudo ... ` command into the CI scripts is ideal
Yeah, they should be fine to run as a user normally. The only dependencies are Python, Bash, and a container engine.
💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3082824189)
> This means a new pull request should hit a _pretty relevant_ cache.
> Old pull requests **which are not being rebased on master** may suffer from lower cache hit-rate.
I don't think this is true. A pull request that modifies a core header (like serialize.h) will now always start from a cold cache. The current persistent workers have a high ccache hit rate for pulls that are (force) pushed for minor fixups (https://0xb10c.github.io/bitcoin-core-ci-stats/graph/ccache/). Also, before CI runs,
...
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3082824189)
> This means a new pull request should hit a _pretty relevant_ cache.
> Old pull requests **which are not being rebased on master** may suffer from lower cache hit-rate.
I don't think this is true. A pull request that modifies a core header (like serialize.h) will now always start from a cold cache. The current persistent workers have a high ccache hit rate for pulls that are (force) pushed for minor fixups (https://0xb10c.github.io/bitcoin-core-ci-stats/graph/ccache/). Also, before CI runs,
...
🤔 naiyoma reviewed a pull request: "validation: remove BLOCK_FAILED_CHILD"
(https://github.com/bitcoin/bitcoin/pull/32950#pullrequestreview-3028174620)
Concept ACK,
Wondering if the CI failure is related to this issue. ->https://github.com/bitcoin/bitcoin/issues/32855
(https://github.com/bitcoin/bitcoin/pull/32950#pullrequestreview-3028174620)
Concept ACK,
Wondering if the CI failure is related to this issue. ->https://github.com/bitcoin/bitcoin/issues/32855
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3082859342)
> > This means a new pull request should hit a _pretty relevant_ cache.
> > Old pull requests **which are not being rebased on master** may suffer from lower cache hit-rate.
>
> I don't think this is true. A pull request that modifies a core header (like serialize.h) will now always start from a cold cache. The current persistent workers have a high ccache hit rate for pulls that are (force) pushed for minor fixups ([0xb10c.github.io/bitcoin-core-ci-stats/graph/ccache](https://0xb10c.github.
...
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3082859342)
> > This means a new pull request should hit a _pretty relevant_ cache.
> > Old pull requests **which are not being rebased on master** may suffer from lower cache hit-rate.
>
> I don't think this is true. A pull request that modifies a core header (like serialize.h) will now always start from a cold cache. The current persistent workers have a high ccache hit rate for pulls that are (force) pushed for minor fixups ([0xb10c.github.io/bitcoin-core-ci-stats/graph/ccache](https://0xb10c.github.
...
💬 maflcko commented on pull request "test: refactor: overhaul block hash determination for `CBlock{,Header}` objects":
(https://github.com/bitcoin/bitcoin/pull/32868#issuecomment-3082914368)
very nice, but it would be good to rebase and fix the ci failure.
review ACK 50b41bcfbfd93f103cf6e2af1999c43947bc3f40 🏋
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+k
...
(https://github.com/bitcoin/bitcoin/pull/32868#issuecomment-3082914368)
very nice, but it would be good to rebase and fix the ci failure.
review ACK 50b41bcfbfd93f103cf6e2af1999c43947bc3f40 🏋
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+k
...