💬 fanquake commented on pull request "guix: GCC 12 consolidation":
(https://github.com/bitcoin/bitcoin/pull/30511#issuecomment-2247215570)
Guix Build (aarch64, x86_64):
```bash
4fba0aba8be59fd967b099e19faf6ba6770a83de1e7bee745c16f91fa3eb92fe guix-build-d1592d2eee19/output/aarch64-linux-gnu/SHA256SUMS.part
37e93c9235db88f58e5a45e8a54e1d57602f9107cd23676c91252509b2fe3bb5 guix-build-d1592d2eee19/output/aarch64-linux-gnu/bitcoin-d1592d2eee19-aarch64-linux-gnu-debug.tar.gz
184fb61be0843b069c4c0d92119b9e29e895f11ef8687b220ab3898999c06afa guix-build-d1592d2eee19/output/aarch64-linux-gnu/bitcoin-d1592d2eee19-aarch64-linux-gnu.tar.gz
...

🚀 fanquake merged a pull request: "locks: introduce mutex for tx download, flush rejection filters once per tip change"
(https://github.com/bitcoin/bitcoin/pull/30111)

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689370440)
I don't think it matters much either way, because it is an internal implementation detail. It doesn't need access to private or protected members, so standalone seems better. Otherwise, there could be confusion when calling `base_blob<160>::FromHex<uint256>(hex)`, as to what values are used. Also, if the function is changed to access the private or protected members, it may be moved to the cpp file, or otherwise "optimized", so spending too much time here now is probably not worth it.

Happy t
...

💬 hodlinator commented on pull request "refactor: Add consteval uint256("str") and ParseHex("str")":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1689373817)
Thanks! That is a piece of art. :)

(Think I would put any `Assert()`s and `vector` initialization inside of `RuntimeParse()`).

🚀 fanquake merged a pull request: "contrib: simplify `test-security-check`"
(https://github.com/bitcoin/bitcoin/pull/30423)

💬 hodlinator commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689387173)
Hm.. yeah, to me the namespace itself is a cognitive speed-bump but I see your point about keeping to the public members.

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

```
bitcoin-tx.cpp:288:23: error: non-constant-expression cannot be narrowed from type 'int64_t' (aka 'long') to 'uint32_t' (aka 'unsigned int') in initializer list [-Wc++11-narrowing]
288 | CTxIn txin{*txid, vout, CScript(), nSequenceIn};
| ^~~~
bitcoin-tx.cpp:288:23: note: insert an explicit cast to silence this issue
288 | CTxIn txin{*txid, vout, CScript(), nSequenceIn};
| ^~~~
|
...

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#issuecomment-2247260119)
Addressed all feedback.

Can be tested by running the new regression test in `test/functional/interface_rest.py` from this pull on current master to see it fail.

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689395871)
Ok, resolving for now. Happy to change if there is a convincing reason.

💬 maflcko commented on pull request "refactor: Add consteval uint256("str") and ParseHex("str")":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1689399364)
Yeah, `RuntimeParse` is probably just `TryParseHex`, which returns a vector already, so you can drop the vector-move-constructor from my suggestion.

💬 hodlinator commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689400559)
Why not have all `FromHex` methods be `[[nodiscard]]`? Seems bad to throw away work.

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689410263)
The attribute should only be added to functions where ignoring the return value will lead to an error later on in the code. See https://github.com/bitcoin/bitcoin/pull/27774#issuecomment-1567120817

Otherwise, if it was added to all functions that return something, it would instead be better to add it to no function and enforce the behavior via a language plugin, for example via clang-tidy.

💬 glozow 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_r1689392956)
nit on naming: I assume you originally named these after the script. Maybe call them `tx_anchor`, `tx_spend_nonempty_wit`, `tx_spend_nested`, etc?

💬 glozow 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_r1689400975)
This isn't testing that mempool accepts a tx creating a p2a, assuming that's what you were going for.
Perhaps we want something like this (passes for me on master as well, as expected):
```suggestion
true_tx = self.wallet.create_self_transfer_multi(sequence=SEQUENCE_FINAL, num_outputs=2)['tx']
true_tx.vout[0].scriptPubKey = bytes(PAY_TO_ANCHOR)
node.sendrawtransaction(true_tx.serialize().hex()
self.generate(node, 1)
```

💬 maflcko commented on pull request "m_tx_download_mutex followups":
(https://github.com/bitcoin/bitcoin/pull/30507#discussion_r1689423926)
style-nit: Generally, when a pointer is unconditionally dereferenced, it is better to use a reference, because it can not be null. This means that any possible undefined behavior, or crash, is visible early on in the call stack and will more likely be caught early during review, instead of only at runtime (when debug logging is active), or not at all.

```suggestion
void ValidationSignals::ActiveTipChange(const CBlockIndex& new_tip, bool is_ibd)
{
LOG_EVENT("%s: new block hash=%s block
...

💬 maflcko commented on pull request "m_tx_download_mutex followups":
(https://github.com/bitcoin/bitcoin/pull/30507#discussion_r1689429226)
note to myself: See if `ACQUIRED_BEFORE` helps here at compile time

💬 hodlinator commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689429761)
In principle all pure functions like `FromHex` (that at most mutate input-parameters and don't have asserts/throw) should be `[[nodiscard]]`. One could at least apply that rule to new code?

💬 vasild commented on pull request "test: fix debug log assertion in p2p_i2p_ports test":
(https://github.com/bitcoin/bitcoin/pull/30345#issuecomment-2247326712)
> Perhaps we could get a random available port for that?

How?

👍 vasild approved a pull request: "net: call `Select` with reachable networks in `ThreadOpenConnections`"
(https://github.com/bitcoin/bitcoin/pull/29436#pullrequestreview-2196096229)
ACK ef525de104ef6116b4532801ef3fcbc1ad5e5552

🤔 hodlinator requested changes to a pull request: "rest: Reject truncated hex txid early in getutxos parsing"
(https://github.com/bitcoin/bitcoin/pull/30482#pullrequestreview-2196099141)
Without seeing an exploratory branch where `SetHexDeprecated` is removed, I think it might be better to call it `SetHexDiscouraged` after all, in case it turns out to be really problematic to remove it for some unforeseen reason.

💬 hodlinator commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689435355)
Should use `BOOST_CHECK_EQUAL_COLLECTIONS()`.