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

💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689477812)
aren't we mixing big and little endian here? Is this by design? As mentioned in the other PR, I think these unintuitive differences should definitely be documented...

💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689480655)
> But happy to remove it, if you think

ok, but seems a bit passive-aggressive to resolve all such comments in that case.

-----

Wouldn't this achieve the same in a more idiomatic way?
```suggestion
auto prev_filter_header_basic{uint256::FromHex(test[pos++].get_str()).value()};
```
This will fails for
```C++
static std::optional<uint256> FromHex(std::string_view str) { return std::nullopt; }
```
with
> std::bad_optional_access: bad_optional_access

which is more specifi
...

💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689463908)
I was asking because `ParseHashStr` returned a `bool` before and `FromHex` returns an `optional`, was wondering if it had some weirdness that I didn't know about (like how JavaScript's truthy has), e.g. take the internal value into consideration in some cases or whatever.
I checked it now (was reviewing the other one from my phone), it seems to behave in the same way indeed.

💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689440058)
I guess it's easy for the compiler to eliminate the call if it's indeed pure and the result isn't used - but even better if there's a warning as well

💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689516659)
+1, gives way better errors on failure

💬 willcl-ark commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#discussion_r1689524038)
I agree that updating the help text (in a followup) would be nice. I think (a slightly modified version of) the explanation from the 0.15.0 release notes would be a good improvement, as IMO it's clearer than wording like "is more likely to be sufficient for the desired target":

```
2. estimate_mode (string, optional, default="economical") The fee estimate mode.
*Conservative* estimates use longer time horizons to produce an estimate which
is less
...

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