Compare COutPoints lexicographically

[josibake]

Broken out from #28122

---

BIP352 requires the smallest output lexicographically, which is my primary motivation for adding this as the default way of compare COutPoints (e.g.

bitcoin/src/common/bip352.cpp

Line 149 in 5b06ccf

auto smallest_outpoint = std::min_element(tx_outpoints.begin(), tx_outpoints.end());

)

Outside of needing this for #28122 , I think it makes more sense to compare COutPoints by their serialization for any use case that needs ordering since the serialization is what appears in the final transaction. I also didn't see any existing places in the code where we order outpoints, so this change doesn't conflict with any current use cases that I can see. If there is a need to order inputs by their vout field (e.g. for an RPC output), something like

std::sort(outpoints.begin(), outpoints.end(), [](const COutPoint& a, const COutPoint& b) { return a.vout < b.vout; });

seems preferable, rather than first comparing a.hash < b.hash.

[DrahtBot]

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

[darosior]

I fail to see how it's not functionally equivalent? (but much less efficient)

[josibake]

See

bitcoin/src/test/data/bip352_send_and_receive_vectors.json

Line 383 in 246e2a2

"comment": "Outpoint ordering byte-lexicographically vs. vout-integer",

for an example of how the old method (a.hash < b.hash, a.vout < b.vout) will yield a different ordering than comparing outpoints lexicographically.

[josibake]

Although, to your point, a.hash < b.hash is doing a lexicographic comparison:

bitcoin/src/uint256.h

Line 54 in e623d8a

constexpr int Compare(const base_blob& other) const { return std::memcmp(m_data.data(), other.m_data.data(), WIDTH); }

So maybe it would be more efficient to just serialize a.vout, b.vout as little-endian here, rather than serializing the whole COutPoint?

[darosior]

But 1 sorts before 256 lexicographically? So it's lexicographical serialized order, i see.

[josibake]

Yep, good point. I added a comment to clarify that this is a lexicographic sort on the serialized outpoint. I think this is preferable to what we were doing before since its based on the protocol definition of an outpoint: <32-byte txid, little-endian>:<4-byte vout, little-endian>.

[Sjors]

Don't forget to add a source comment that BIP352 depends on this exact ordering. Either in this PR or as a commit in #28122.

[josibake]

Don't forget to add a source comment that BIP352 depends on this exact ordering. Either in this PR or as a commit in #28122.

Hm, I don't think we need a BIP352 specific comment here? I'd argue if we are going to sort outpoints, we should be sorting them based on their protocol definition, regardless of BIP352. In fact, when writing BIP352 we already assumed this was the case which is why we defined the outpoint sorting in BIP352 to be done lexicographically on the outpoint serialization. It was only recently that someone pointed out that Bitcoin Core wasn't sorting lexicographically based on the serialization and instead was comparing the txids lexicographically but comparing the vouts as integers.

[sipa]

This seems unnecessarily unnatural and inefficient to use as a default sort order for this type.

If BIP352 defines its own normative sort order, I think it's better to have a specialized BIP352Comparator class that implements it, and use that in the BIP352 code?

That also has the advantage of not tying the two together; someone might come in and change the order back to the current ordering for performance or simplicity reasons, and break the BIP352 code.

[josibake]

This seems unnecessarily unnatural and inefficient to use as a default sort order for this type.

If BIP352 defines its own normative sort order, I think it's better to have a specialized BIP352Comparator class that implements it, and use that in the BIP352 code?

That also has the advantage of not tying the two together; someone might come in and change the order back to the current ordering for performance or simplicity reasons, and break the BIP352 code.

Personally, I find it more confusing that we have three different ways we can compare outpoints, so lexicographic comparison based on the 36-byte serialization seemed the simplest default. Fair points regarding efficiency and not tightly coupling the default sort with a specific use case, though. Happy to close this in favor of a BIP352Comparator.