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

🤔 theStack reviewed a pull request: "wallet: Remove IsMine from migration code"
(https://github.com/bitcoin/bitcoin/pull/30328#pullrequestreview-2196286870)
Concept ACK, left two code-deduplication nits below

💬 theStack commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1689547396)
nit: can use the `RIPEMD160` helper here
```suggestion
uint160 hash{RIPEMD160(sols[0])};
```

💬 theStack commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1689550504)
nit: to deduplicate code, could introduce an `all_keys_compressed` helper that is used here and below in the `is_valid_script` lambda, e.g. something like:

```
const auto& all_keys_compressed = [](const std::vector<valtype>& keys) -> bool {
return std::all_of(keys.cbegin(), keys.cend(),
[](const auto& key) { return key.size() == 33; });
};
```

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689553146)
I am not changing this line of code, or the behavior of this test, so a separate pull request seems more appropriate.

💬 willcl-ark commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#discussion_r1689530964)
nit: if you retouch `Pathlib` lets you use the `/` operator to create child paths:

```suggestion
fee_dat = self.nodes[0].chain_path / "fee_estimates.dat"
```

🤔 willcl-ark reviewed a pull request: "Fee Estimation: change `estimatesmartfee` default mode to `economical`"
(https://github.com/bitcoin/bitcoin/pull/30275#pullrequestreview-2196260920)
Looks good to me, left a micro style nit if you end up touching to address @glozow 's comments.

It makes sense to default to more economical fee estimation by default, since users can use RBF (or "conservative" estimation) in those cases when it may be required. RBF opt-in has been enabled by default in Bitcoin Core wallets since ~2022 and there is a proposal to enable "full" RBF as default as mempool policy currently open: https://github.com/bitcoin/bitcoin/pull/30493 (previously https://gi
...