🤔 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

💬 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) {
```

👍 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.

💬 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?

💬 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. */
```

💬 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());
```

💬 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/`

💬 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
...

👍 stickies-v approved a pull request: "log: Enforce trailing newline"
(https://github.com/bitcoin/bitcoin/pull/30929#pullrequestreview-2338066360)
ACK fa6444fe628fc33b77d45773c13b27a5a557cd2f , I much prefer this new approach with less code complexity and a more user-friendly logging interface, that allows for a gradual transition to not having `\n`-terminated log statements.

I wonder if it would make more sense to switch the commit order so the `bitcoin-tidy` check is removed after auto-appending the newline, instead of before?

As a quick belt-and-suspenders way to check that there's no behaviour change, I ran a `-debug=all` node fo
...

💬 stickies-v commented on pull request "log: Enforce trailing newline":
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1781470805)
nit: I think this comment should now also be removed: https://github.com/bitcoin/bitcoin/blob/f3c74c4a7e120ea363abe4d4aa034b85c1d71919/src/logging.h#L256

💬 stickies-v commented on pull request "log: Enforce trailing newline":
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1781444088)
Suppression for this file can be cleaned up now:

<details>
<summary>git diff on fa6444fe62</summary>

```diff
diff --git a/test/lint/lint-format-strings.py b/test/lint/lint-format-strings.py
index a809851ec6..86a17fb0f8 100755
--- a/test/lint/lint-format-strings.py
+++ b/test/lint/lint-format-strings.py
@@ -62,7 +62,7 @@ def main():

matching_files_filtered = []
for matching_file in matching_files:
- if not re.search('^src/(leveldb|secp256k1|minisketc
...

💬 stickies-v commented on pull request "[28.x] backports and finalize":
(https://github.com/bitcoin/bitcoin/pull/30959#issuecomment-2383735956)
It seems to have been the process for 26.0 and 27.0 at least:

- https://github.com/bitcoin/bitcoin/pull/29780/files#diff-e04cb1e9f29f76897c6b84334238f621ff45ba13cf180ad1d0d442d50ebe51f7
- https://github.com/bitcoin/bitcoin/pull/28959/files#diff-e04cb1e9f29f76897c6b84334238f621ff45ba13cf180ad1d0d442d50ebe51f7

💬 achow101 commented on pull request "docs: Add instructions on how to self-sign bitcoin-core binaries for macOS":
(https://github.com/bitcoin/bitcoin/pull/30982#issuecomment-2383736840)
Should also add this to the current release notes draft: https://github.com/bitcoin-core/bitcoin-devwiki/wiki/28.0-Release-Notes-Draft

✅ torkelrogstad closed a pull request: "rpc: validate fee estimation mode case insensitive"
(https://github.com/bitcoin/bitcoin/pull/29175)

💬 torkelrogstad commented on pull request "rpc: validate fee estimation mode case insensitive":
(https://github.com/bitcoin/bitcoin/pull/29175#issuecomment-2383747798)
No, not really. Feel free to pick it up and get it in a mergeable state.

💬 brunoerg commented on issue "docs: "Fuzzing the Bitcoin Core P2P layer using Honggfuzz NetDriver" is outdated":
(https://github.com/bitcoin/bitcoin/issues/30957#issuecomment-2383748628)
> Yes I think so but similar to the PoW check it never makes sense to have this check in the way. The p2p_transport_serialization harness currently has [a bunch of extra logic](https://github.com/bitcoin/bitcoin/blob/d812cf11896a2214467b6fa72d7b763bac6077c5/src/test/fuzz/p2p_transport_serialization.cpp#L43-L44) for reaching the unhappy/happy paths of these checks. We can achieve the same with the macro (without having to create valid checksum in the test), e.g.:

Looks good to me!

💬 torkelrogstad commented on pull request "doc: update signet documentation related to build directories":
(https://github.com/bitcoin/bitcoin/pull/30996#discussion_r1781498059)
This is the build directory used by the [unix](https://github.com/bitcoin/bitcoin/blob/master/doc/build-unix.md#to-build) and [macOS](https://github.com/bitcoin/bitcoin/blob/master/doc/build-osx.md#2-compile) build docs. IMO it's therefore not necessary to mention that here. If people are clever enough to change the build directories they'll also understand that this changes the location of binaries.

💬 achow101 commented on pull request "[28.x] backports and finalize":
(https://github.com/bitcoin/bitcoin/pull/30959#issuecomment-2383758464)
Hmm ok, will add those soon. Still some changes to be made on the release notes.

💬 pablomartin4btc commented on pull request "doc: update signet documentation related to build directories":
(https://github.com/bitcoin/bitcoin/pull/30996#discussion_r1781504068)
ok, just to be consistent on other places in the documentation where this is clarified

💬 jonatack commented on issue "V2 Only Option":
(https://github.com/bitcoin/bitcoin/issues/29618#issuecomment-2383823827)
> @jonatack I gotta say, looking up the background of the author on a pretty ordinary feature request feels a bit creepy to me.

@petertodd The rationale was mentioned in my comment, but to iterate on it, the proposal came out of left field from an unknown entity who insisted repeatedly in an odd manner on it. Also, the proposal may be potentially dangerous for the network (cf https://github.com/bitcoin/bitcoin/issues/29618#issuecomment-1994415042 and https://github.com/bitcoin/bitcoin/pull/30
...