🤔 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
...
💬 maflcko commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2212508710)
the typo wasn't fixed in the recent force pushes, fwiw
(https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2212508710)
the typo wasn't fixed in the recent force pushes, fwiw
💬 Sjors commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#issuecomment-3083004175)
> will look into fixing the CI job
Just add a dummy commit so the subtree merge is more than 6 deep :-)
(https://github.com/bitcoin/bitcoin/pull/28122#issuecomment-3083004175)
> will look into fixing the CI job
Just add a dummy commit so the subtree merge is more than 6 deep :-)
📝 maflcko opened a pull request: "ci: Use APT_LLVM_V in msan task"
(https://github.com/bitcoin/bitcoin/pull/32999)
This skips compilation of clang by using the apt.
(https://github.com/bitcoin/bitcoin/pull/32999)
This skips compilation of clang by using the apt.
💬 Sjors commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#issuecomment-3083204938)
I tried to use this to boost the silent payment indexer, but I don't know what I'm doing :-) https://github.com/Sjors/bitcoin/pull/96
(https://github.com/bitcoin/bitcoin/pull/26966#issuecomment-3083204938)
I tried to use this to boost the silent payment indexer, but I don't know what I'm doing :-) https://github.com/Sjors/bitcoin/pull/96
💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2212634650)
nit in https://github.com/bitcoin/bitcoin/commit/2d0ac71899eadcbff8252b1704d2affb73f0160d: Why not just `git rev-parse HEAD:depends`, like it was done for cirrus? (See `git log -S 'git rev-parse HEAD:depends'`)
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2212634650)
nit in https://github.com/bitcoin/bitcoin/commit/2d0ac71899eadcbff8252b1704d2affb73f0160d: Why not just `git rev-parse HEAD:depends`, like it was done for cirrus? (See `git log -S 'git rev-parse HEAD:depends'`)