👍 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.
💬 naumenkogs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1793800548)
2658d2b2dfaacbfb2ddf2f1305ed8c3c9e134b71
won't this fail in `DIFFERENT_WITNESS` case?
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1793800548)
2658d2b2dfaacbfb2ddf2f1305ed8c3c9e134b71
won't this fail in `DIFFERENT_WITNESS` case?
💬 maflcko commented on pull request "bench: Adds a benchmark for CheckInputScripts":
(https://github.com/bitcoin/bitcoin/pull/29745#issuecomment-2402770771)
Closing for now due to inactivity since the day it was opened. Leave a comment below if you want this re-opened, or open a new pull.
(https://github.com/bitcoin/bitcoin/pull/29745#issuecomment-2402770771)
Closing for now due to inactivity since the day it was opened. Leave a comment below if you want this re-opened, or open a new pull.
✅ maflcko closed a pull request: "validation: Use witness maleation flag for non-segwit blocks"
(https://github.com/bitcoin/bitcoin/pull/29540)
(https://github.com/bitcoin/bitcoin/pull/29540)
💬 maflcko commented on pull request "validation: Use witness maleation flag for non-segwit blocks":
(https://github.com/bitcoin/bitcoin/pull/29540#issuecomment-2402780104)
Closing for now due to inactivity (lack of reply to a question about the motivation for this change). Please leave a comment, if you want this re-opened.
(https://github.com/bitcoin/bitcoin/pull/29540#issuecomment-2402780104)
Closing for now due to inactivity (lack of reply to a question about the motivation for this change). Please leave a comment, if you want this re-opened.
🤔 stickies-v reviewed a pull request: "refactor: Check translatable format strings at compile-time"
(https://github.com/bitcoin/bitcoin/pull/31061#pullrequestreview-2357070300)
Concept ACK, and this is a smaller diff than I'd expected to implement this, nice!
I need to think about it more and work out some code, but I feel like the new interface is not as intuitive yet as it could be. Will follow up with that in next review.
(https://github.com/bitcoin/bitcoin/pull/31061#pullrequestreview-2357070300)
Concept ACK, and this is a smaller diff than I'd expected to implement this, nice!
I need to think about it more and work out some code, but I feel like the new interface is not as intuitive yet as it could be. Will follow up with that in next review.
💬 stickies-v commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793488009)
in aaaa4fb20156b4375d92e1eca4acc90a425a1896: seems more straightforward to just extend `ConstevalStringLiteral` instead of creating a new `util::Original`? This would 1) minimize the diff and 2) I find `ConstevalStringLiteral` a much more helpful name than `Original`.
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793488009)
in aaaa4fb20156b4375d92e1eca4acc90a425a1896: seems more straightforward to just extend `ConstevalStringLiteral` instead of creating a new `util::Original`? This would 1) minimize the diff and 2) I find `ConstevalStringLiteral` a much more helpful name than `Original`.
💬 stickies-v commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793707636)
```suggestion
QMessageBox::information(m_parent_widget, tr("Restore wallet message"), QString::fromStdString("Wallet restored successfully \n"));
```
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793707636)
```suggestion
QMessageBox::information(m_parent_widget, tr("Restore wallet message"), QString::fromStdString("Wallet restored successfully \n"));
```
💬 stickies-v commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793744937)
This function is currently unused, and it seems incorrectly implemented as it doesn't compile when actually used:
```
In file included from ../../src/clientversion.cpp:9:
../../src/util/translation.h:42:48: error: too many arguments to function call, expected 0, have 1
std::string translate() { return translate(original.fmt); }
~~~~~~~~~ ^~~~~~~~~~~~
../../src/util/translation.h:106:42: note: in instantiation of member function 'util::bilingual_fm
...
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793744937)
This function is currently unused, and it seems incorrectly implemented as it doesn't compile when actually used:
```
In file included from ../../src/clientversion.cpp:9:
../../src/util/translation.h:42:48: error: too many arguments to function call, expected 0, have 1
std::string translate() { return translate(original.fmt); }
~~~~~~~~~ ^~~~~~~~~~~~
../../src/util/translation.h:106:42: note: in instantiation of member function 'util::bilingual_fm
...
💬 stickies-v commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793473436)
typo nit
```suggestion
/** Type to denote whether an original string literal is translatable */
```
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793473436)
typo nit
```suggestion
/** Type to denote whether an original string literal is translatable */
```
💬 stickies-v commented on pull request "refactor: Check translatable format strings at compile-time":
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793829614)
nit: I would prefer templating this on an enum `{Translateable,Untranslateable}` for readability over the non-obvious `<{true,false}>` boolean
(https://github.com/bitcoin/bitcoin/pull/31061#discussion_r1793829614)
nit: I would prefer templating this on an enum `{Translateable,Untranslateable}` for readability over the non-obvious `<{true,false}>` boolean
💬 kevkevinpal commented on pull request "bench: Adds a benchmark for CheckInputScripts":
(https://github.com/bitcoin/bitcoin/pull/29745#issuecomment-2402804271)
> Closing for now due to inactivity since the day it was opened. Leave a comment below if you want this re-opened, or open a new pull.
ya sorry about that I can leave it up for grabs if anyone else wants to look at it otherwise I can reopen when I have a more finalized PR open, again sorry for leaving it open but inactive
(https://github.com/bitcoin/bitcoin/pull/29745#issuecomment-2402804271)
> Closing for now due to inactivity since the day it was opened. Leave a comment below if you want this re-opened, or open a new pull.
ya sorry about that I can leave it up for grabs if anyone else wants to look at it otherwise I can reopen when I have a more finalized PR open, again sorry for leaving it open but inactive
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2402903171)
@Sjors
I cannot reproduce the issue on macOS Intel w/o Xcode you mentioned in:
- https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2388568119
- https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2396235386
On my MacBook Pro (2019, Intel) + macOS 15.0.1, no Xcode, I can successfully build 86837bc75c4c1282fc652dfd755273db956222db as follows:
```
% brew list
==> Formulae
cmake ninja pkg-config
==> Casks
% make -j 16 -C depends NO_WALLET=1 NO_UPNP=1 NO_ZMQ=1
% cm
...
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2402903171)
@Sjors
I cannot reproduce the issue on macOS Intel w/o Xcode you mentioned in:
- https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2388568119
- https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2396235386
On my MacBook Pro (2019, Intel) + macOS 15.0.1, no Xcode, I can successfully build 86837bc75c4c1282fc652dfd755273db956222db as follows:
```
% brew list
==> Formulae
cmake ninja pkg-config
==> Casks
% make -j 16 -C depends NO_WALLET=1 NO_UPNP=1 NO_ZMQ=1
% cm
...