💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686055997)
With "fragile" I mean that it breaks code that uses it and leads to bugs. See for example the bug that was fixed in this pull request.

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686056295)
Good point. I'll just remove this comment.

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

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686057241)
I am not touching this line of code, so it seems unrelated to refactor it.

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686057721)
Happy to review a pull request doing this, but I won't be doing it myself.

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686060040)
I am not changing the endian behavior in this instance, so maybe a separate pull can improve the documentation?

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686061163)
See https://github.com/bitcoin/bitcoin/pull/30482#issuecomment-2241616078

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686061254)
See https://github.com/bitcoin/bitcoin/pull/30482#issuecomment-2241616078

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686065219)
> What is your reasoning for deprecating rather than removing and correcting call-sites

I don't think fixing all bugs in all places in a single pull request makes sense. Often, context specific fixes are needed, so separate pull requests can be used.

> is that part of why this PR is a draft?

It is draft, because it includes a merge commit of other commits.



> If you keep it, `TxidFromString` should probably be renamed `TxidFromStringFragile` to follow `SetHexFragile`

Sure, but
...

💬 maflcko commented on pull request "fuzz: Limit parse_univalue input length":
(https://github.com/bitcoin/bitcoin/pull/30473#discussion_r1686075826)
No, because the fuzz engine will think that the trimming code is then relevant to the fuzz target. Thus, it will treat coverage feedback in the trimming code as useful and bloat the fuzz input set. Skipping it should be effective to avoid that, apart from https://github.com/bitcoin/bitcoin/pull/27552 which should be a "complete" fix. But this can be done later.

💬 maflcko commented on pull request "refactor: Add consteval uint256("str") and ParseHex("str")":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1686109312)
Maybe `BytesFromHex`?

💬 maflcko commented on pull request "refactor: Add consteval uint256("str") and ParseHex("str")":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1686115679)
nit: snake_case for new code: `write_it`, `str_it`, according to the dev notes.

💬 maflcko commented on pull request "logging: Replace LogError and LogWarning with LogAlert":
(https://github.com/bitcoin/bitcoin/pull/30364#discussion_r1686133882)
> I think this should probably be `LogDebug` rather than `Alert` messages?

It may be good to check that at least one warning is emitted to the user. Otherwise, it may be harder for them to spot the config error at all? (However, if non-logging related changes are made, I wonder if such changes can be done in separate (preparatory) pull requests, similar to https://github.com/bitcoin/bitcoin/pull/30064, as they require in-depth low-level net knowledge?)

💬 fjahr commented on pull request "validation: assumeutxo params mainnet":
(https://github.com/bitcoin/bitcoin/pull/28553#issuecomment-2242330336)
tACK f193b0fd853cb9e2f7cb4ed9dd6fb5d2662a04fe

I have re-tested the full assumeutxo flow using this snapshot: Downloaded the snapshot from the torrent link, verified the hash, successfully loaded the snapshot in a fresh node, verified that the snapshot chainstate was able to sync to the tip and the node was usable. Then waited until the background chainstate caught up to the snapshot and that was also successful. Restarted the node and verified the cleanup of the second chainstate. Also checke
...

💬 maflcko commented on pull request "rpc: doc: use "output script" terminology consistently in "asm"/"hex" results":
(https://github.com/bitcoin/bitcoin/pull/30408#discussion_r1686138362)
> `witness program` or `output script` seems like it might be more accurate than `witness output script`.

No. A witness program is different from the witness output script. The witness output script consists of both, the "version byte" and the "witness program". You can refer to BIP 141 for more details: https://en.bitcoin.it/wiki/BIP_0141#Witness_program

💬 maflcko commented on pull request "fuzz: reduce keypool size in `scriptpubkeyman` target":
(https://github.com/bitcoin/bitcoin/pull/30494#issuecomment-2242360509)
review ACK dcb4ec944984961e3b40452425a219e15e6ab508

This implements my suggestion from https://github.com/bitcoin/bitcoin/issues/30476#issue-2415683883, but I haven't benchmarked whether it works.

💬 maflcko commented on pull request "qa: Functional test improvements":
(https://github.com/bitcoin/bitcoin/pull/30463#issuecomment-2242372890)
Discussions are resolved and this seems rfm with two reviews?

💬 t-bast commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2242378141)
> Stealing an ephemeral anchor output can only be done in certain conditions, at specific ranges of values. It is not always profitable due to overheads, and you can reduce those overheads by doing complex things.

I don't understand this hand-wavy comment, can you detail? I don't see any complexity here. Whenever a miner would include a transaction spending an ephemeral output in a block:

- if that ephemeral output is economical, the miner replaces the spending transaction by one that send
...

✅ Sjors closed a pull request: "Introduce waitFeesChanged() mining interface"
(https://github.com/bitcoin/bitcoin/pull/30443)

💬 Sjors commented on pull request "Introduce waitFeesChanged() mining interface":
(https://github.com/bitcoin/bitcoin/pull/30443#issuecomment-2242382625)
I'm moving this to https://github.com/Sjors/bitcoin/pull/52.

Since it's astronomically unlikely multiprocess will be in the upcoming v28.0 release, we have at least half a year to flesh out this interface. Hopefully cluster mempool will be further along by then, since that will inform the design.