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

🤔 glozow reviewed a pull request: "Fee Estimation: change `estimatesmartfee` default mode to `economical`"
(https://github.com/bitcoin/bitcoin/pull/30275#pullrequestreview-2196180471)
Nice test! I just have a few comments

💬 glozow commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#discussion_r1689486484)
Why don't we want the nodes to sync? We can't expect the fee estimates to be updated until the blocks have been received.

💬 glozow commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#discussion_r1689502072)
The test still works if no target weight is specified, and I can't think of why it would be necessary - is there a need for it? Otherwise, removing that + minimizing the extra config options would be preferable.

💬 glozow commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#discussion_r1689495566)
This comment seems wrong - `broadcast_and_mine` generates 3 blocks at a time, so this is 36 blocks not 12?

💬 glozow commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#discussion_r1689502353)
Why wait=0.1?

💬 glozow commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#discussion_r1689506756)
nit: this could be more future-proof and self-documenting:

```suggestion
fee_est_conservative = node.estimatesmartfee(1, estimate_mode="conservative")['feerate']
fee_est_economical = node.estimatesmartfee(1, estimate_mode="economical")['feerate']
fee_est_default = node.estimatesmartfee(1)['feerate']
assert_equal(fee_est_conservative, expected_conservative)
assert_equal(fee_est_economical, expected_economical)
assert_equal(fee_est_default, expected_economical)
``
...

💬 hodlinator commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689510417)
Yeah, sorry, `BOOST_CHECK_EQUAL()`. Seems to be a mis-merge on your part when rebasing. Considering I took the time to add a whole extra commit to convert away from `BOOST_CHECK` in this file recently, I would be quicker to ACK if you fixed the merge.

💬 hodlinator commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#issuecomment-2247483093)
> I appreciate the feedback, but when it comes to trivial style nits, I don't consider it worth it to spend days on it myself.

Fair.

💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689437033)
nit: we're not using SetHexDeprecated here, right?
```suggestion
BOOST_AUTO_TEST_CASE(methods) // GetHex FromHex begin() end() size() GetLow64 GetSerializeSize, Serialize, Unserialize
```

💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689441061)
nit: the conditions in the code are actually reversed

💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689446176)
as hitned before, this seems like a slight behavior change: previously if `txid_out.at(1)` was invalid, `txid_out.at(0)` wasn't checked. But I guess it's just extra work at most, no side-effects should follow from running `IsHex`, right?