💬 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?
🤔 jonatack reviewed a pull request: "refactor: ensure type safety for txid and wtxid in `RelayTransaction`"
(https://github.com/bitcoin/bitcoin/pull/31001#pullrequestreview-2338097406)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31001#pullrequestreview-2338097406)
Concept ACK
💬 jonatack commented on pull request "refactor: ensure type safety for txid and wtxid in `RelayTransaction`":
(https://github.com/bitcoin/bitcoin/pull/31001#discussion_r1781456973)
`tx` only needed inside the conditional
```diff
- CTransactionRef tx = m_mempool.get(txid);
-
- if (tx != nullptr) {
+ if (CTransactionRef tx = m_mempool.get(txid)) {
```
or
```diff
+ if (CTransactionRef tx = m_mempool.get(txid); tx != nullptr) {
```
(https://github.com/bitcoin/bitcoin/pull/31001#discussion_r1781456973)
`tx` only needed inside the conditional
```diff
- CTransactionRef tx = m_mempool.get(txid);
-
- if (tx != nullptr) {
+ if (CTransactionRef tx = m_mempool.get(txid)) {
```
or
```diff
+ if (CTransactionRef tx = m_mempool.get(txid); tx != nullptr) {
```
👍 instagibbs approved a pull request: "cluster mempool: extend DepGraph functionality"
(https://github.com/bitcoin/bitcoin/pull/30857#pullrequestreview-2338044262)
reACK 7051fcdda6fdc95677a34d5c14321d7580eceed8
via `git range-diff master aab53ddcd8fcbc3c0be0da9383f8e06abe5badda 7051fcdda6fdc95677a34d5c14321d7580eceed8`
New fuzzer harness is a nice generalization of what existed prior.
(https://github.com/bitcoin/bitcoin/pull/30857#pullrequestreview-2338044262)
reACK 7051fcdda6fdc95677a34d5c14321d7580eceed8
via `git range-diff master aab53ddcd8fcbc3c0be0da9383f8e06abe5badda 7051fcdda6fdc95677a34d5c14321d7580eceed8`
New fuzzer harness is a nice generalization of what existed prior.
💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1781424806)
`clusterlin: merge several DepGraph fuzz tests into simulation test`
Seems to only merge 2 fwiw, might just enumerate them?
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1781424806)
`clusterlin: merge several DepGraph fuzz tests into simulation test`
Seems to only merge 2 fwiw, might just enumerate them?
💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1781428797)
```Suggestion
/** Read any set of transactions from the provider including empty slots. */
```
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1781428797)
```Suggestion
/** Read any set of transactions from the provider including empty slots. */
```
💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1781444249)
In case somehow the sim had no non-empty slots above and misses the `idx == i` assert
```Suggestion
// Update sim.
assert(!sim[i].has_value());
```
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1781444249)
In case somehow the sim had no non-empty slots above and misses the `idx == i` assert
```Suggestion
// Update sim.
assert(!sim[i].has_value());
```
💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1781436150)
mu-nit: `s/num_tx/sim_num_tx/`
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1781436150)
mu-nit: `s/num_tx/sim_num_tx/`
💬 stickies-v commented on pull request "log: Enforce trailing newline":
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1781439184)
Would be nice to add test coverage to prevent regressions? Imo can be as simple as slightly modifying the existing `logging_LogPrintMacros` test. E.g. this would fail on fae943042f38012ec3410cc8988928896e924352:
<details>
<summary>git diff on fae943042f</summary>
```diff
diff --git a/src/test/logging_tests.cpp b/src/test/logging_tests.cpp
index 8217a2385c..9cfdf8293a 100644
--- a/src/test/logging_tests.cpp
+++ b/src/test/logging_tests.cpp
@@ -135,8 +135,8 @@ BOOST_FIXTURE_TEST_CASE(l
...
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1781439184)
Would be nice to add test coverage to prevent regressions? Imo can be as simple as slightly modifying the existing `logging_LogPrintMacros` test. E.g. this would fail on fae943042f38012ec3410cc8988928896e924352:
<details>
<summary>git diff on fae943042f</summary>
```diff
diff --git a/src/test/logging_tests.cpp b/src/test/logging_tests.cpp
index 8217a2385c..9cfdf8293a 100644
--- a/src/test/logging_tests.cpp
+++ b/src/test/logging_tests.cpp
@@ -135,8 +135,8 @@ BOOST_FIXTURE_TEST_CASE(l
...