💬 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

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

💬 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_r1689545924)
I actually didn't mean to test it, but makes sense to in case by defining the output we somehow made it non-standard. Taken

💬 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_r1689546084)
renamed things

💬 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_r1689546146)
done, with even fewer lines. Nice to reduce the boilerplate a bit.

💬 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_r1689547347)
I actually just re-used `WitnessUnknown`'s own operators since they'll work just fine.

✅ hebasto closed an issue: "Fuzzing related configuration/build options can be improved"
(https://github.com/bitcoin/bitcoin/issues/30318)

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689552375)
> ok, but seems a bit passive-aggressive to resolve all such comments in that case.

I am sorry you feel that way, but I don't see value in review comments or questions that can be answered by the person asking the question. (`Assert` is pretty well documented, see `git grep Assert doc/developer-notes.md`)

I did answer the question and then resolved the conversation, which seems reasonable for a question.

> which is more specific than

I think both are fine. As long as the test fails i
...