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

💬 hodlinator commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689446576)
There's only 1 remaining explicit call to `SetHexDeprecated` in this test case, before there were 7 calls to `SetHex`. But I guess almost the same tests are done indirectly in the `basics` test case above where `SetHexDeprecated` is being called via `uint<N>S`.

🚀 fanquake merged a pull request: "[27.x] More backports"
(https://github.com/bitcoin/bitcoin/pull/30467)

💬 glozow commented on pull request "m_tx_download_mutex followups":
(https://github.com/bitcoin/bitcoin/pull/30507#discussion_r1689461711)
Makes sense. Should I add this to the followup?

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689468576)
> 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?

If there is a convincing reason to do this, the rule should be added to the dev notes. However, absent a convincing reason and absent the style rule being in the dev notes, I will leave this as-is for now.

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689476302)
Why? This blob of code is untouched in this pull request, so if there is a reason to change it, it should be done in a separate pull request.

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#issuecomment-2247429330)
> 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.

Thank you for the suggestion. However, outside of `src/qt`, and internally, the function is used in a single place `uint256S`, so the name shouldn't matter much. There is already a lengthy bike-shedding thread about the naming in https://github.com/bitcoin/bitcoin/
...

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689484353)
Ok, previously it was `BOOST_CHECK_EQUAL(TmpL, uint256());`. I may change it to that, if I have to re-touch for other reasons. Otherwise, it can be done in one of the follow-ups.