💬 maflcko commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#issuecomment-2402335504)
> However, I think it could be simplified significantly if there were a scripted diff commit to replace `strprintf(Untranslated("..."), ...)` with `Untranslated(strprintf("...", ...))`, which would be a good change on its own for clarity and consistency.
Are you sure? The two are not equivalent, because args can also be a `bilingual_str`, in which case the scripted-diff does not compile. In any case, it seems unrelated to this pull request and I don't see how it "simplifies" stuff and it touc
...
(https://github.com/bitcoin/bitcoin/pull/31061#issuecomment-2402335504)
> However, I think it could be simplified significantly if there were a scripted diff commit to replace `strprintf(Untranslated("..."), ...)` with `Untranslated(strprintf("...", ...))`, which would be a good change on its own for clarity and consistency.
Are you sure? The two are not equivalent, because args can also be a `bilingual_str`, in which case the scripted-diff does not compile. In any case, it seems unrelated to this pull request and I don't see how it "simplifies" stuff and it touc
...
💬 maflcko commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793530260)
I don't think this is possible with a simple diff touching only lines that are already touched in this pull request. If you disagree, I am happy to accept any commit that compiles. I am also happy to review any pull request that achieves this and is split up from this pull request.
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793530260)
I don't think this is possible with a simple diff touching only lines that are already touched in this pull request. If you disagree, I am happy to accept any commit that compiles. I am also happy to review any pull request that achieves this and is split up from this pull request.
💬 maflcko commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793533834)
> It seems like it could be a avoided with a small scripted diff to replace `strprintf(Untranslated("..."), ...)` with `Untranslated(strprintf("...", ...))` and that replacement would make the code clearer and more consistent anyway.
Are you sure? The two are not equivalent, because args can also be a `bilingual_str`, in which case the scripted-diff does not compile. In any case, it seems unrelated to this pull request and I don't see how it "simplifies" stuff and it touches different lines a
...
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793533834)
> It seems like it could be a avoided with a small scripted diff to replace `strprintf(Untranslated("..."), ...)` with `Untranslated(strprintf("...", ...))` and that replacement would make the code clearer and more consistent anyway.
Are you sure? The two are not equivalent, because args can also be a `bilingual_str`, in which case the scripted-diff does not compile. In any case, it seems unrelated to this pull request and I don't see how it "simplifies" stuff and it touches different lines a
...
💬 maflcko commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793534710)
(same)
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793534710)
(same)
💬 maflcko commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793543713)
Thanks, fixed commit message.
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793543713)
Thanks, fixed commit message.
💬 maflcko commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793544349)
Thanks, added unrelated change to fix comment.
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793544349)
Thanks, added unrelated change to fix comment.
💬 brunoerg commented on pull request "fuzz: wallet: add target for `CreateTransaction`":
(https://github.com/bitcoin/bitcoin/pull/29936#issuecomment-2402403167)
review ping @maflcko
(https://github.com/bitcoin/bitcoin/pull/29936#issuecomment-2402403167)
review ping @maflcko
💬 Xaspr commented on issue "Unable to sync blockchain on laptop: ERROR: ReadBlockFromDisk: Deserialize or I/O error":
(https://github.com/bitcoin/bitcoin/issues/29255#issuecomment-2402454601)
Ah yes, I'll look into whether the lower load does anything.
Just to be complete on what happened. The IBD continued after restarting Core again, but after an eventual third shutdown and restart of Core to test what would happen, the block database was corrupted again.
```
2024-10-09T13:52:42Z UpdateTip: new best=00000000000000000878cd8de3dccda91cdcf6ef0e6e8536120c65f487dd0197 height=390296 version=0x00000004 log2_work=83.803743 tx=100238753 date='2015-12-26T14:25:06Z' progress=0.09181
...
(https://github.com/bitcoin/bitcoin/issues/29255#issuecomment-2402454601)
Ah yes, I'll look into whether the lower load does anything.
Just to be complete on what happened. The IBD continued after restarting Core again, but after an eventual third shutdown and restart of Core to test what would happen, the block database was corrupted again.
```
2024-10-09T13:52:42Z UpdateTip: new best=00000000000000000878cd8de3dccda91cdcf6ef0e6e8536120c65f487dd0197 height=390296 version=0x00000004 log2_work=83.803743 tx=100238753 date='2015-12-26T14:25:06Z' progress=0.09181
...
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2402462147)
@maflcko
How do I clean depends sources cache on the Cirrus CI for this PR?
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2402462147)
@maflcko
How do I clean depends sources cache on the Cirrus CI for this PR?
💬 ajtowns commented on pull request "CAT in Tapscript (BIP-347)":
(https://github.com/bitcoin/bitcoin/pull/29247#discussion_r1793618993)
Added to the inquisition PR in https://github.com/bitcoin-inquisition/bitcoin/pull/68
(https://github.com/bitcoin/bitcoin/pull/29247#discussion_r1793618993)
Added to the inquisition PR in https://github.com/bitcoin-inquisition/bitcoin/pull/68
💬 maflcko commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2402496447)
They'd have to be cleared on all machines. Let me known if you want that to happen. If you are just looking for a temporary workaround for testing, you can pick a different qt version, like `6.7.1`. Or you can temporarily use a new sources volume, similar to the diff in https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2337816419.
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2402496447)
They'd have to be cleared on all machines. Let me known if you want that to happen. If you are just looking for a temporary workaround for testing, you can pick a different qt version, like `6.7.1`. Or you can temporarily use a new sources volume, similar to the diff in https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2337816419.
💬 maflcko commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793642022)
> Also (not sure about this, but) it might be nice to add an explicit conversion method to Original class, so we could write something like `_("Error opening block database").str()` instead of `bilingual_str{_("Error opening block database")}`
Sure, I am happy to add `bilingual_str str()` and `operator bilingual_str()` to the `Original` type and remove the constructors from `bilingual_str`. However, this means that calling `bilingual_str{Original{}}` is now forbidden and one would have to use
...
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793642022)
> Also (not sure about this, but) it might be nice to add an explicit conversion method to Original class, so we could write something like `_("Error opening block database").str()` instead of `bilingual_str{_("Error opening block database")}`
Sure, I am happy to add `bilingual_str str()` and `operator bilingual_str()` to the `Original` type and remove the constructors from `bilingual_str`. However, this means that calling `bilingual_str{Original{}}` is now forbidden and one would have to use
...
👍 maflcko approved a pull request: "fuzz: wallet: add target for `CreateTransaction`"
(https://github.com/bitcoin/bitcoin/pull/29936#pullrequestreview-2357490000)
ACK c495731a316d9c97ee05a08cf5087c5535f84bd4 🏻
<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+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK c495731a316d9c97ee05a08cf5
...
(https://github.com/bitcoin/bitcoin/pull/29936#pullrequestreview-2357490000)
ACK c495731a316d9c97ee05a08cf5087c5535f84bd4 🏻
<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+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK c495731a316d9c97ee05a08cf5
...
💬 maflcko commented on pull request "fuzz: wallet: add target for `CreateTransaction`":
(https://github.com/bitcoin/bitcoin/pull/29936#discussion_r1793727754)
nit in c495731a316d9c97ee05a08cf5087c5535f84bd4: starting with C++17, you can use `try_emplace`:
```diff
diff --git a/src/wallet/test/fuzz/spend.cpp b/src/wallet/test/fuzz/spend.cpp
index 9abd9e402a..cd092256c1 100644
--- a/src/wallet/test/fuzz/spend.cpp
+++ b/src/wallet/test/fuzz/spend.cpp
@@ -68,7 +68,7 @@ FUZZ_TARGET(wallet_create_transaction, .init = initialize_setup)
tx.vout[0].scriptPubKey = GetScriptForDestination(fuzzed_wallet.GetDestination(fuzzed_data_provider));
...
(https://github.com/bitcoin/bitcoin/pull/29936#discussion_r1793727754)
nit in c495731a316d9c97ee05a08cf5087c5535f84bd4: starting with C++17, you can use `try_emplace`:
```diff
diff --git a/src/wallet/test/fuzz/spend.cpp b/src/wallet/test/fuzz/spend.cpp
index 9abd9e402a..cd092256c1 100644
--- a/src/wallet/test/fuzz/spend.cpp
+++ b/src/wallet/test/fuzz/spend.cpp
@@ -68,7 +68,7 @@ FUZZ_TARGET(wallet_create_transaction, .init = initialize_setup)
tx.vout[0].scriptPubKey = GetScriptForDestination(fuzzed_wallet.GetDestination(fuzzed_data_provider));
...
💬 mzumsande commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2402684547)
> I haven't looked closer into why these happen.
Two mechanisms have been pointed out above, both are due to timing issues in orphan resolving: https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2273685985 and https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2274310940 - I wouldn't be surprised if there were others.
Obviously resolving these are blockers for any further progress, whether with a BIP or not. If this PR was deployed as is, any two upgraded peers would discon
...
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2402684547)
> I haven't looked closer into why these happen.
Two mechanisms have been pointed out above, both are due to timing issues in orphan resolving: https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2273685985 and https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2274310940 - I wouldn't be surprised if there were others.
Obviously resolving these are blockers for any further progress, whether with a BIP or not. If this PR was deployed as is, any two upgraded peers would discon
...
💬 hebasto commented on issue "macOS 15.0.1 can't build Qt (Intel, without Xcode)":
(https://github.com/bitcoin/bitcoin/issues/31054#issuecomment-2402733760)
I cannot neither confirm nor reproduce the issue on my MacBook Pro (2019, Intel).
The following scenarios were tested:
- Sonoma 14.6.1 + Command-Line Tools 15.0.
- Sonoma 14.7 + Command-Line Tools 16.0.
- Sequoia 15.0.1 + Command-Line Tools 16.0.
- Sequoia 15.0.1 + Xcode 16.0.
- Sequoia 15.0.1 + Xcode 16.0 installed and then deleted.
For all scenarios, here is the list of the installed Homebrew's packages:
```
% brew list
==> Formulae
cmake pkg-config
==> Casks
```
> But i
...
(https://github.com/bitcoin/bitcoin/issues/31054#issuecomment-2402733760)
I cannot neither confirm nor reproduce the issue on my MacBook Pro (2019, Intel).
The following scenarios were tested:
- Sonoma 14.6.1 + Command-Line Tools 15.0.
- Sonoma 14.7 + Command-Line Tools 16.0.
- Sequoia 15.0.1 + Command-Line Tools 16.0.
- Sequoia 15.0.1 + Xcode 16.0.
- Sequoia 15.0.1 + Xcode 16.0 installed and then deleted.
For all scenarios, here is the list of the installed Homebrew's packages:
```
% brew list
==> Formulae
cmake pkg-config
==> Casks
```
> But i
...
📝 maflcko opened a pull request: "lint: commit-script-check.sh: echo to stderr"
(https://github.com/bitcoin/bitcoin/pull/31063)
This makes it easier to redirect the produced `git diff` on failure. On success, it shouldn't hurt, because the same output is still present, just on stderr.
Can be tested by introducing a fault in any scripted diff and then calling `commit-script-check.sh HEAD~..HEAD > any_file.txt`. Previously the file contained the full output, now it contains just the diff.
(https://github.com/bitcoin/bitcoin/pull/31063)
This makes it easier to redirect the produced `git diff` on failure. On success, it shouldn't hurt, because the same output is still present, just on stderr.
Can be tested by introducing a fault in any scripted diff and then calling `commit-script-check.sh HEAD~..HEAD > any_file.txt`. Previously the file contained the full output, now it contains just the diff.
💬 maflcko commented on pull request "doc: cmake: prepend "build" to functional/test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/30859#issuecomment-2402756550)
lgtm ACK c15ef096ce317ae911dd756593adeac9d9a0b5a1
(https://github.com/bitcoin/bitcoin/pull/30859#issuecomment-2402756550)
lgtm ACK c15ef096ce317ae911dd756593adeac9d9a0b5a1
✅ maflcko closed a pull request: "bench: Adds a benchmark for CheckInputScripts"
(https://github.com/bitcoin/bitcoin/pull/29745)
(https://github.com/bitcoin/bitcoin/pull/29745)
💬 theuni commented on pull request "RFC: build: support for pre-compiled headers.":
(https://github.com/bitcoin/bitcoin/pull/31053#issuecomment-2402769455)
> > Combining with #30911 produces even more of a speedup (with Make, ninja is about the same).
>
> Why are ninja builds not affected?
Ninja builds are sped up significantly by pch (this PR), it's the combo with #30911 doesn't seem to have much effect.
(https://github.com/bitcoin/bitcoin/pull/31053#issuecomment-2402769455)
> > Combining with #30911 produces even more of a speedup (with Make, ninja is about the same).
>
> Why are ninja builds not affected?
Ninja builds are sped up significantly by pch (this PR), it's the combo with #30911 doesn't seem to have much effect.