💬 glozow 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_r1689400975)
This isn't testing that mempool accepts a tx creating a p2a, assuming that's what you were going for.
Perhaps we want something like this (passes for me on master as well, as expected):
```suggestion
true_tx = self.wallet.create_self_transfer_multi(sequence=SEQUENCE_FINAL, num_outputs=2)['tx']
true_tx.vout[0].scriptPubKey = bytes(PAY_TO_ANCHOR)
node.sendrawtransaction(true_tx.serialize().hex()
self.generate(node, 1)
```

💬 maflcko commented on pull request "m_tx_download_mutex followups":
(https://github.com/bitcoin/bitcoin/pull/30507#discussion_r1689423926)
style-nit: Generally, when a pointer is unconditionally dereferenced, it is better to use a reference, because it can not be null. This means that any possible undefined behavior, or crash, is visible early on in the call stack and will more likely be caught early during review, instead of only at runtime (when debug logging is active), or not at all.

```suggestion
void ValidationSignals::ActiveTipChange(const CBlockIndex& new_tip, bool is_ibd)
{
LOG_EVENT("%s: new block hash=%s block
...

💬 maflcko commented on pull request "m_tx_download_mutex followups":
(https://github.com/bitcoin/bitcoin/pull/30507#discussion_r1689429226)
note to myself: See if `ACQUIRED_BEFORE` helps here at compile time

💬 hodlinator commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689429761)
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?

💬 vasild commented on pull request "test: fix debug log assertion in p2p_i2p_ports test":
(https://github.com/bitcoin/bitcoin/pull/30345#issuecomment-2247326712)
> Perhaps we could get a random available port for that?

How?

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