💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1781298290)
Probably something that should be mentioned in the commit message? Also in the PR description, if you're adding the requirement that building for a certain platform will now require the very latest release of Qt.
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1781298290)
Probably something that should be mentioned in the commit message? Also in the PR description, if you're adding the requirement that building for a certain platform will now require the very latest release of Qt.
💬 instagibbs commented on pull request "refactor: ensure type safety for txid and wtxid in `RelayTransaction`":
(https://github.com/bitcoin/bitcoin/pull/31001#issuecomment-2383456576)
@dergoegge that's an argument for bigger bites then?
Not against these or more changes.
(https://github.com/bitcoin/bitcoin/pull/31001#issuecomment-2383456576)
@dergoegge that's an argument for bigger bites then?
Not against these or more changes.
💬 marcofleon commented on pull request "refactor: ensure type safety for txid and wtxid in `RelayTransaction`":
(https://github.com/bitcoin/bitcoin/pull/31001#issuecomment-2383468176)
I'll look to expand this one a bit then. I also think the suggestion to add more context is a good one. That way I can refer back to this PR on future ones as a way to track progress.
(https://github.com/bitcoin/bitcoin/pull/31001#issuecomment-2383468176)
I'll look to expand this one a bit then. I also think the suggestion to add more context is a good one. That way I can refer back to this PR on future ones as a way to track progress.
💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1781330901)
In 9ad6801174298241cfeff80c5ba9214b4bb9981f (ci: Update for Qt 6):
I was under the impression that these kinds of GUI-only warnings should be suppressed in the build system, i.e:
https://github.com/bitcoin/bitcoin/blob/f3c74c4a7e120ea363abe4d4aa034b85c1d71919/src/qt/CMakeLists.txt#L126 ? I think we should do one or the other, but not spread supression between the build system and the CI.
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1781330901)
In 9ad6801174298241cfeff80c5ba9214b4bb9981f (ci: Update for Qt 6):
I was under the impression that these kinds of GUI-only warnings should be suppressed in the build system, i.e:
https://github.com/bitcoin/bitcoin/blob/f3c74c4a7e120ea363abe4d4aa034b85c1d71919/src/qt/CMakeLists.txt#L126 ? I think we should do one or the other, but not spread supression between the build system and the CI.
👍 instagibbs approved a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2337805760)
reACK ce205abe10ebccfdbea60adf4c568a8ba61390c3
via `git range-diff master 1d4e33e7f88162cf6fcd6aee0b63973015853591 ce205abe10ebccfdbea60adf4c568a8ba61390c3`
new unit test coverage looks superior, and catches the move error previously encountered.
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2337805760)
reACK ce205abe10ebccfdbea60adf4c568a8ba61390c3
via `git range-diff master 1d4e33e7f88162cf6fcd6aee0b63973015853591 ce205abe10ebccfdbea60adf4c568a8ba61390c3`
new unit test coverage looks superior, and catches the move error previously encountered.
💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1781291525)
Do you mean it cannot be in both, and result in a stored orphan?
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1781291525)
Do you mean it cannot be in both, and result in a stored orphan?
💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1781283774)
could assert that new idx doesn't overflow the `m_coinbase_txns` vector size
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1781283774)
could assert that new idx doesn't overflow the `m_coinbase_txns` vector size
💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1781293825)
```Suggestion
// Send all coins to 1 output.
```
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1781293825)
```Suggestion
// Send all coins to 1 output.
```
💬 maflcko commented on pull request "test: switch MiniWallet padding unit from weight to vsize":
(https://github.com/bitcoin/bitcoin/pull/30718#issuecomment-2383519968)
re-ACK 940edd6ac24e921f639b1376fe7d35dc14c1887d 🍷
<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: re-ACK 940edd6ac24e921f639b
...
(https://github.com/bitcoin/bitcoin/pull/30718#issuecomment-2383519968)
re-ACK 940edd6ac24e921f639b1376fe7d35dc14c1887d 🍷
<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: re-ACK 940edd6ac24e921f639b
...
👍 ryanofsky approved a pull request: "log: Enforce trailing newline"
(https://github.com/bitcoin/bitcoin/pull/30929#pullrequestreview-2337909567)
Code review ACK fa6444fe628fc33b77d45773c13b27a5a557cd2f. Nice simplification removing dead code after #30485.
(https://github.com/bitcoin/bitcoin/pull/30929#pullrequestreview-2337909567)
Code review ACK fa6444fe628fc33b77d45773c13b27a5a557cd2f. Nice simplification removing dead code after #30485.
💬 ryanofsky commented on issue "RFC: Multiprocess binaries and packaging options":
(https://github.com/bitcoin/bitcoin/issues/30983#issuecomment-2383562384)
> @ryanofsky bin/bitcoin should (or could) be meta-process of all others that you mentioned. Just symlink maybe?
It could be a symlink to existing executable, which could interpret its arguments as needed based on argv[0], instead of being a new executable. But I think having a new executable that calls exec https://man7.org/linux/man-pages/man3/exec.3.html would be more straightforward
(https://github.com/bitcoin/bitcoin/issues/30983#issuecomment-2383562384)
> @ryanofsky bin/bitcoin should (or could) be meta-process of all others that you mentioned. Just symlink maybe?
It could be a symlink to existing executable, which could interpret its arguments as needed based on argv[0], instead of being a new executable. But I think having a new executable that calls exec https://man7.org/linux/man-pages/man3/exec.3.html would be more straightforward
💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1781392510)
A few more assertions here and other spot for MempoolRejectedTx?
```
bool first_time_failure{fuzzed_data_provider.ConsumeBool()};
bool reject_contains_wtxid{txdownload_impl.RecentRejectsFilter().contains(rand_tx->GetWitnessHash().ToUint256())};
node::RejectedTxTodo todo = txdownload_impl.MempoolRejectedTx(rand_tx, state, rand_peer, first_time_failure);
Assert(first_time_failure || !todo.m_should_add_extra_compact_tx);
...
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1781392510)
A few more assertions here and other spot for MempoolRejectedTx?
```
bool first_time_failure{fuzzed_data_provider.ConsumeBool()};
bool reject_contains_wtxid{txdownload_impl.RecentRejectsFilter().contains(rand_tx->GetWitnessHash().ToUint256())};
node::RejectedTxTodo todo = txdownload_impl.MempoolRejectedTx(rand_tx, state, rand_peer, first_time_failure);
Assert(first_time_failure || !todo.m_should_add_extra_compact_tx);
...
💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1781367693)
Ideally this would select N inputs rather than a static one, otherwise this target really only covers multi-input case with the `// 2 parents 1 child` case, and doesn't even cover duplicate prevout hashes.
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1781367693)
Ideally this would select N inputs rather than a static one, otherwise this target really only covers multi-input case with the `// 2 parents 1 child` case, and doesn't even cover duplicate prevout hashes.
💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1781349142)
If no outpoints are given, it's no inputs at all? I think every call is using non-empty vectors, and if so should be asserted.
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1781349142)
If no outpoints are given, it's no inputs at all? I think every call is using non-empty vectors, and if so should be asserted.
💬 achow101 commented on pull request "[28.x] backports and finalize (or rc3)":
(https://github.com/bitcoin/bitcoin/pull/30959#issuecomment-2383600137)
> I think `doc/release-notes.md` still needs to be updated though?
I think that's usually done when after the release is published on the website.
(https://github.com/bitcoin/bitcoin/pull/30959#issuecomment-2383600137)
> I think `doc/release-notes.md` still needs to be updated though?
I think that's usually done when after the release is published on the website.
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2383601118)
@fanquake
> It'd be good if [49b0256](https://github.com/bitcoin/bitcoin/commit/49b025636be266fa17cbb4cdb9d541c0e71f2ef8) explained why bumping to 12 is needed, as the Qt 6 supported platforms for Linux claims anything down to GCC 9 should be supported: https://doc.qt.io/qt-6/supported-platforms.html#linux-x11. I'm guessing this is just because we are using 12 to compile the bins, and could also be worked around.
I agree with your guess. Do you have any hints on possible workarounds?
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2383601118)
@fanquake
> It'd be good if [49b0256](https://github.com/bitcoin/bitcoin/commit/49b025636be266fa17cbb4cdb9d541c0e71f2ef8) explained why bumping to 12 is needed, as the Qt 6 supported platforms for Linux claims anything down to GCC 9 should be supported: https://doc.qt.io/qt-6/supported-platforms.html#linux-x11. I'm guessing this is just because we are using 12 to compile the bins, and could also be worked around.
I agree with your guess. Do you have any hints on possible workarounds?
🤔 jonatack reviewed a pull request: "build: Switch to Qt 6"
(https://github.com/bitcoin/bitcoin/pull/30997#pullrequestreview-2338041473)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30997#pullrequestreview-2338041473)
Concept ACK
💬 jonatack commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1781423174)
Would the minimum QT version in `doc/dependencies.md` need to be updated?
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1781423174)
Would the minimum QT version in `doc/dependencies.md` need to be updated?
🤔 pablomartin4btc reviewed a pull request: "doc: update signet documentation related to build directories"
(https://github.com/bitcoin/bitcoin/pull/30996#pullrequestreview-2338051388)
ACK a647d4400d55558735f102988e84eedc39b3affa
(https://github.com/bitcoin/bitcoin/pull/30996#pullrequestreview-2338051388)
ACK a647d4400d55558735f102988e84eedc39b3affa
💬 pablomartin4btc commented on pull request "doc: update signet documentation related to build directories":
(https://github.com/bitcoin/bitcoin/pull/30996#discussion_r1781430078)
nit:
```suggestion
GRIND="./build/src/bitcoin-util grind" #assuming the build directory is named `build`
```
(https://github.com/bitcoin/bitcoin/pull/30996#discussion_r1781430078)
nit:
```suggestion
GRIND="./build/src/bitcoin-util grind" #assuming the build directory is named `build`
```