💬 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'`)
👍 maflcko approved a pull request: "Migrate CI to hosted Cirrus Runners"
(https://github.com/bitcoin/bitcoin/pull/32989#pullrequestreview-3028393275)
I guess you want review here and then address it, as it comes in? Once review is finished, the app will be installed and reviewers can also look at a "real" run in this repo?
looked at c0ad2b6aa8e8c31c9f9c9ea2b35ca86f7985c490~23 🎬
<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_t
...
(https://github.com/bitcoin/bitcoin/pull/32989#pullrequestreview-3028393275)
I guess you want review here and then address it, as it comes in? Once review is finished, the app will be installed and reviewers can also look at a "real" run in this repo?
looked at c0ad2b6aa8e8c31c9f9c9ea2b35ca86f7985c490~23 🎬
<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_t
...
💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2212678980)
nit in the same commit: Could use `github.event.repository.default_branch` instead of `master`, so that caches can be saved for other repos, such as inquisition?
(same below)
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2212678980)
nit in the same commit: Could use `github.event.repository.default_branch` instead of `master`, so that caches can be saved for other repos, such as inquisition?
(same below)