💬 marcofleon commented on pull request "refactor: ensure type safety for txid and wtxid in `RelayTransaction`":
(https://github.com/bitcoin/bitcoin/pull/31001#issuecomment-2383391992)
> is this in view of some follow-on work or a one-off?
I noticed it while doing a bit of review on Erlay (https://github.com/bitcoin/bitcoin/pull/30116).
> If the standalone chunks are too small and this is split-up too much, the review overhead may be higher than needed to make meaningful progress here?
Agreed that it may be too small a change to warrant a PR. I was trying to keep the scope within `RelayTransaction` only. But I can do some more investigation and see if it makes sense
...
(https://github.com/bitcoin/bitcoin/pull/31001#issuecomment-2383391992)
> is this in view of some follow-on work or a one-off?
I noticed it while doing a bit of review on Erlay (https://github.com/bitcoin/bitcoin/pull/30116).
> If the standalone chunks are too small and this is split-up too much, the review overhead may be higher than needed to make meaningful progress here?
Agreed that it may be too small a change to warrant a PR. I was trying to keep the scope within `RelayTransaction` only. But I can do some more investigation and see if it makes sense
...
💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1781281974)
In 1b95937fb70b5f5ec7462d48018b7180cc37c1e1 (Fix compiling for Windows):
From what I can tell, `QModernWindowsStylePlugin` was [introduced in Qt `6.7.0`](https://github.com/qt/qtbase/commit/65d58e6c41e3c549c89ea4f05a8e467466e79ca3), which I assume means this change drags the minimum required Qt for Windows up to `6.7.0` (the current latest release)? Is that correct?
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1781281974)
In 1b95937fb70b5f5ec7462d48018b7180cc37c1e1 (Fix compiling for Windows):
From what I can tell, `QModernWindowsStylePlugin` was [introduced in Qt `6.7.0`](https://github.com/qt/qtbase/commit/65d58e6c41e3c549c89ea4f05a8e467466e79ca3), which I assume means this change drags the minimum required Qt for Windows up to `6.7.0` (the current latest release)? Is that correct?
💬 dergoegge commented on pull request "refactor: ensure type safety for txid and wtxid in `RelayTransaction`":
(https://github.com/bitcoin/bitcoin/pull/31001#issuecomment-2383422769)
> Easier to swallow these useful pills if the code is going to have churn anyways
I understand where this is coming from but if we only do these refactors if we're already touching the code, then I don't think we'll ever finish the migration (there is simply too much code to be touched). I'd prefer for us to move to the new types as soon as possible and remove the `const uint256&` conversion function, so that we end up with actual type-safety.
I suggested to Marco to work on this, so that
...
(https://github.com/bitcoin/bitcoin/pull/31001#issuecomment-2383422769)
> Easier to swallow these useful pills if the code is going to have churn anyways
I understand where this is coming from but if we only do these refactors if we're already touching the code, then I don't think we'll ever finish the migration (there is simply too much code to be touched). I'd prefer for us to move to the new types as soon as possible and remove the `const uint256&` conversion function, so that we end up with actual type-safety.
I suggested to Marco to work on this, so that
...
💬 ryanofsky commented on pull request "log: Enforce trailing newline":
(https://github.com/bitcoin/bitcoin/pull/30929#issuecomment-2383429828)
Approach ACK fadce8893d766a9ead7493c1a00a885066156b00. Two things:
- Would be good to clarify in the description that the reason why "This refactor does not change behavior" is that the only unterminated log prints were removed in #30485, so there is no code relying on the old behavior. (Assuming this is the case)
- I wonder if it would be possible to simplify the implementation by just making the newline part of log formatting:
```diff
--- a/src/logging.cpp
+++ b/src/logging.cpp
@@
...
(https://github.com/bitcoin/bitcoin/pull/30929#issuecomment-2383429828)
Approach ACK fadce8893d766a9ead7493c1a00a885066156b00. Two things:
- Would be good to clarify in the description that the reason why "This refactor does not change behavior" is that the only unterminated log prints were removed in #30485, so there is no code relying on the old behavior. (Assuming this is the case)
- I wonder if it would be possible to simplify the implementation by just making the newline part of log formatting:
```diff
--- a/src/logging.cpp
+++ b/src/logging.cpp
@@
...
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1781294860)
Correct.
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1781294860)
Correct.
💬 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.