The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32621.

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #32116 (contrib: refactor: dedup deserialization routines in utxo-to-sqlite script by theStack)
• #31560 (rpc: allow writing UTXO set to a named pipe, introduce dump_to_sqlite.sh script by theStack)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

• if --spk=raw, then scriptpubkey will be BLOB instead. -> If --spk=raw, then scriptpubkey will be BLOB instead. [sentence start should be capitalized]

No other typos were found.

^{drahtbot_id_4_m}

The approach taken is introducing a -m/--mode parameter which can either have the values "hex" (default) or "bytes". Happy to take suggestions on naming and thoughts on future extensibility etc.

I wonder if separate options would be better, eg --spk=raw --txid=raw?

I think there is no ambiguity on byte-string-serialization of txids; they are ultimately just hash results and hence, they should be stored as such, and adding a big/little endian knob wouldn't make much sense.

I think it will be surprising if hex(txid) gives "reversed" values so that just copy and pasting into block explorers doesn't work. If you did separate flags, you could consider --txid=hex, --txid=raw (same byte order as hex), and --txid=rawhash (same byte order as sha256 output) as separate options, maybe?

Here's a patch: ajtowns@d1fc5df

Could consider allowing --spk=none --txid=none to drop both fields for further space saving if you're just analysing the utxo set, not doing things with it (or --txid=counter to replace the actual txid with a counter that saves space but still lets you notice when utxos are from the same tx).

The drawback of not being able to immediately show txid-strings (as one would need to do the bytes-reversal step first, which is not possible in sqlite, see e.g. #24952 (comment)) still remains though.

Seems like you can get addons for this, for whatever that's worth.

@ajtowns: Thanks for your suggestions, I like the idea of having more flexibility and mostly agree with the naming. Took your patch (TIL about argparse choices) and adapted the functional test accordingly, to exercise all combinations of new options. As for txid format option naming, I think I'd intuitively prefer raw for storing them in the hash order, since that's AFAICT the most common txid byte-string serialization (I think I haven't encountered any counter-examples so far in the Bitcoin ecosystem) -- but then one would need to pick a good name for the "reverse-hash" byte-string serialization again. So maybe it's just fine 🤷‍♂️ Personally, with the current naming, I'd only either wanted to use the combinations --txid=hex --spk=hex (i.e. the default, the only possible on master), or --txid=rawhash --spk=raw.

Could consider allowing --spk=none --txid=none to drop both fields for further space saving if you're just analysing the utxo set, not doing things with it (or --txid=counter to replace the actual txid with a counter that saves space but still lets you notice when utxos are from the same tx).

Pretty neat ideas, and potentially useful for e.g. dust (and other "spam") or output script type analysis. Will consider for further follow-ups.

🚧 At least one of the CI tasks failed.
_{Task lint: https://github.com/bitcoin/bitcoin/runs/42967430443
LLM reason (✨ experimental): The CI failure is caused by a linting error due to a Python f-string missing placeholders.}

I think I'd intuitively prefer raw for storing them in the hash order, since that's AFAICT the most common txid byte-string serialization

Maybe raw and rawle then? For future consideration --spk=type so that it only records whether the spk was p2pk, p2pkh, p2sh, p2wpkh, p2wsh, p2tr, p2a, or other would probably be interesting.

I think I'd intuitively prefer raw for storing them in the hash order, since that's AFAICT the most common txid byte-string serialization

Maybe raw and rawle then?

raw/rawle are a decent alternative that I'd slightly prefer over rawhash/raw, yes! Force-pushed accordingly. I'm fine with both though, so if other reviewers / potential users of that script feel strongly on rawhash/raw, I'm happy to revert.

For future consideration --spk=type so that it only records whether the spk was p2pk, p2pkh, p2sh, p2wpkh, p2wsh, p2tr, p2a, or other would probably be interesting.

Concept ACK. I think there is room for some bike-shedding on details, like e.g. do we split up p2pk into uncompressed and compressed (or, god forbid, even hybrid?) or whether bare multisig should also be a category (if at all, then probably only with a very weak detection like "first byte is in range OP_1...OP_16, last byte is OP_CHECKMULTISIG", as implementing the full detection as used in Solver seems overblown for a conversion script). But, something to discuss in another PR anyways.

Conflicts with #32116 ; let's get that merged first?

Approach ACK