💬 theStack commented on pull request "policy: Add PayToAnchor(P2A), `OP_1 <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1687218507)
nit: these two lines could just be removed, as the solutions array's purpose is to contain the extracted non-fixed parts of an output script. For a P2A output, no such parts exist, and there is imho no value in returning constants.

💬 instagibbs commented on pull request "policy: Add PayToAnchor(P2A), `OP_1 <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1687314098)
done

💬 davidgumberg commented on pull request "net: additional disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#issuecomment-2244097764)
Re-reviewed and manually tested ACK https://github.com/bitcoin/bitcoin/commit/45958d595b70222c89c25768f357a2ebbafac2b1

It should be easy to grep for the reason that a peer got disconnected, and logging the peer's address when `fLogIPs` seems helpful.

Tested by running:

```bash
./bitcoind -datadir=/tmp/trash -debug=net -logips
```

made `fNetworkActive == false` by:

```bash
./bitcoin-cli -datadir=/tmp/trash setnetworkactive false
```

<details>

<summary>Got the following ou
...

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687328626)
added some documentation

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687328664)
done

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687328677)
switched to txid since that's how it's being tracked

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687328696)
documented

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1687328715)
done

💬 davidgumberg commented on pull request "net: additional disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1687338802)
nit: Maybe this should move to the top of the function and be used in the other three disconnect logs here
```diff
if (!ShouldRunInactivityChecks(node, now)) return false;
+const std::string disconnect_msg{...};
```

💬 instagibbs commented on pull request "policy: Add PayToAnchor(P2A), `OP_1 <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1687367154)
ended up adding it since its required if we aren't returning the "solutions" vector

💬 ajtowns commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1687409670)
The memusage is incremented later when `str_prefixed` is included in `buf`, so afaics this should already be accurate.

💬 maflcko commented on issue "Fuzzing related configuration/build options can be improved":
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2244366554)
> Which are those flags?

`DABORT_ON_FAILED_ASSUME` right now, but there can be more in the future. See for example https://duckduckgo.com/?q=FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION . So having a build system flag to enable all of them is useful. Otherwise, if more fuzz-recommended build flags are added in the future, it will be a global breaking change again.

💬 maflcko commented on pull request "fuzz: Speed up PickValue in txorphan":
(https://github.com/bitcoin/bitcoin/pull/30474#discussion_r1687510289)
Thanks, done!

💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687538297)
@paplorinc I see `hi`/`lo` is consistent with `secp256k1_uint128`, so I retract my `high` thoughts.

It strikes me that the `hi` part of your suggestion is not the one being shifted `<< 4`. Maybe it should be called `lo`?

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687543213)
> it parses and makes decisions (e.g. trim, skip values after an invalid char),

No, it does not trim or skip values. Either is sets the internal state, or it does not.

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687550108)
Thanks, fixed the comment in this test!

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1687558959)
Nice, removed!

💬 paplorinc commented on pull request "fuzz: Speed up PickValue in txorphan":
(https://github.com/bitcoin/bitcoin/pull/30474#issuecomment-2244466029)
utACK fa030ad6b6c124f332cea071e16c1519ae7ea423

(could you please add me as co-author, `l0rinc <pap.lorinc@gmail.com>`)?

💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687574482)
> the name of the method hints at mutation, but it actually returns a copy of the view.

This isn't changed. The function signature is identical in this branch and in current master. Both take a copy of the view and return a copy of the view.

> so the copy of the parameter is implicit

No, from reading the function signature, one knows that a copy is being made when the argument is put in as a function parameter. Also, adding a `const` here does not avoid a copy or make a copy explicit in
...

💬 paplorinc commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1687577476)
you're absolutely right, edited the example!