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

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689578142)
Thanks for the feedback, but I don't see the benefit of changing this, so I'll leave it 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_r1689580744)
> no side-effects should follow from running `IsHex`, right?

Yes, IsHex is pure.

📝 fanquake opened a pull request: "ci: add `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES` to TSAN (libc++) job"
(https://github.com/bitcoin/bitcoin/pull/30519)
Add `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES` to one of the libc++ CI jobs, to catch missing includes, that are otherwise hidden by transitive includes inside libc++. A more appropriate place for this might be the tidy job, but that does not use libc++.

See https://libcxx.llvm.org/DesignDocs/HeaderRemovalPolicy.html for more information.

🤔 danielabrozzoni reviewed a pull request: "refactor: Add FlatFileSeq member variables in BlockManager"
(https://github.com/bitcoin/bitcoin/pull/30517#pullrequestreview-2196466308)
ACK 7aa8994c6fceae5cf8fb7e661371cdb19d2cb482

💬 glozow commented on pull request "m_tx_download_mutex followups":
(https://github.com/bitcoin/bitcoin/pull/30507#discussion_r1689635561)
done

💬 glozow commented on pull request "m_tx_download_mutex followups":
(https://github.com/bitcoin/bitcoin/pull/30507#discussion_r1689635405)
Hm, added a `ACQUIRED_BEFORE`, but compiler didn't complain when I added a `LOCK2(m_mempool.cs, m_tx_download_mutex)`

💬 glozow commented on pull request "m_tx_download_mutex followups":
(https://github.com/bitcoin/bitcoin/pull/30507#discussion_r1689635780)
done

👋 glozow's pull request is ready for review: "m_tx_download_mutex followups"
(https://github.com/bitcoin/bitcoin/pull/30507)

💬 maflcko commented on pull request "ci: add `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES` to TSAN (libc++) job":
(https://github.com/bitcoin/bitcoin/pull/30519#discussion_r1689634589)
nit: The same can be achieved, by setting C++23, according to the docs.

This may be beneficial for other reasons as well, even if it is just turning existing compile warnings into errors (https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1675775175)

So adding `-std=c++23` here, and a comment that `libc++` is required for the feature seems better. But this is fine either way.