💬 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`
```
💬 pablomartin4btc commented on pull request "rpc: validate fee estimation mode case insensitive":
(https://github.com/bitcoin/bitcoin/pull/29175#issuecomment-2383661740)
@torkelrogstad, are you still working on this?
(https://github.com/bitcoin/bitcoin/pull/29175#issuecomment-2383661740)
@torkelrogstad, are you still working on this?
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1781439595)
Apologies, this was a remnant from one of the development branches and has been deleted
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1781439595)
Apologies, this was a remnant from one of the development branches and has been deleted
💬 petertodd commented on issue "V2 Only Option":
(https://github.com/bitcoin/bitcoin/issues/29618#issuecomment-2383678063)
@jonatack I gotta say, looking up the background of the author on a pretty ordinary feature request feels a bit creepy to me.
Anyway, it seems reasonable to have V2-only purely on privacy grounds. Nodes that turn that on voluntarily would, modulo traffic analysis, be "black boxes" to passive surveillance attackers, improving the privacy of BTC for everyone else. Don't need to make that the default any time soon. But good if a non-trivial number of people can and do turn it on.
Also, V2-ove
...
(https://github.com/bitcoin/bitcoin/issues/29618#issuecomment-2383678063)
@jonatack I gotta say, looking up the background of the author on a pretty ordinary feature request feels a bit creepy to me.
Anyway, it seems reasonable to have V2-only purely on privacy grounds. Nodes that turn that on voluntarily would, modulo traffic analysis, be "black boxes" to passive surveillance attackers, improving the privacy of BTC for everyone else. Don't need to make that the default any time soon. But good if a non-trivial number of people can and do turn it on.
Also, V2-ove
...
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1781443860)
Thanks! Updated.
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1781443860)
Thanks! Updated.
💬 Christewart commented on issue "Release Schedule for 28.0":
(https://github.com/bitcoin/bitcoin/issues/29891#issuecomment-2383697715)
Tagging is scheduled for today, perhaps we want to sneak in #30982 if it isn't too late?
(https://github.com/bitcoin/bitcoin/issues/29891#issuecomment-2383697715)
Tagging is scheduled for today, perhaps we want to sneak in #30982 if it isn't too late?