Assumeutxo: Altered txoutset dump is still valid

fjahr commented at 3:55 pm on October 18, 2023: contributor

Demonstration code: https://github.com/fjahr/bitcoin/commit/d6ced70903d9d49b8acdfe48a97b6f991b50cc17 (building on top of #28669, the file hashing and additional logging were just added as sanity checks)

It seems that changing certain content of a Txoutset dump doesn’t change the fact that it is considered valid and that the UTXO set is loaded correctly. This doesn’t have to be bad since the hash of the UTXO set still matches, but it was new to me that two different dump files could both be valid and result in the same UTXO set.

I have not been able to figure out yet why exactly this is the case though. From my understanding, the byte I am changing is the Coin VARINT((coinbase ? 1 : 0) | (height << 1)). The original value of the byte is \x82, implying it’s just the value in this one byte. Changing it should result in a different UTXO set. I must be missing something about the serialization of Coin or the dumptxoutset in general…

theStack commented at 9:10 am on October 19, 2023: contributor

After digging deeper here I found something rather unpleasant. The behaviour is caused by a sneaky bug (introduced in #12737 / commit 34ca75032012562d226b06ef9e86a2948d3a8d16) in the serialized UTXO set hash calculation (ApplyHash), which doesn’t commit to the height and coinbase information of coins due to missing parantheses: https://github.com/bitcoin/bitcoin/blob/655dc716aa6043613171a1338e22928de89a7d3e/src/kernel/coinstats.cpp#L77 (=> this expression is always resulting in ss << VARINT(1) for nHeight > 0)

This needs to be fixed for AssumeUTXO, as otherwise modified snapshots where coin’s heights / coinbase flags have been altered would obviously still be imported without error.

The fix in the hash calculation function is trivial, but the question is how to cope with gettxoutsets hash_serialized_2 result. I guess we need to introduce another hash result type (e.g. hash_serialized_3, the bugfixed version of hash_serialized_2) and use that for AssumeUTXO, but still want to keep hash_serialized_2 around for a while to avoid confusing users? Otherwise, it would be odd if the result on the same UTXO set is different on pre-v26 and v26+ nodes.

fanquake commented at 9:23 am on October 19, 2023: member

around for a while to avoid confusing users? Otherwise, it would be odd if the result on the same UTXO set is different on pre-v26 and v26+ nodes.

It’s not odd if the result from the previous version was incorrect. If it doesn’t work, it should be fixed, and any broken version removed, especially if it’s usage going forward would result in invalid assumeutxo usage.

fjahr commented at 9:27 am on October 19, 2023: contributor

If it doesn’t work, it should be fixed

I agree, I don’t think there is any critical usage of hash_serialized_2 currently (other than the future use of assumeutxo) and I have spent quite some time looking into this when I introduced coinstatsindex.

fanquake added this to the milestone 26.0 on Oct 19, 2023

maflcko added the label Bug on Oct 19, 2023

maflcko added the label RPC/REST/ZMQ on Oct 19, 2023

jamesob commented at 1:15 pm on October 19, 2023: member

Wow, good find @fjahr et al. I wonder if there’s any value in committing to a sha256sum of the snapshot file itself in the source code as a belt-and-suspenders remediation for issues like this.

fjahr commented at 7:29 pm on October 19, 2023: contributor

I wonder if there’s any value in committing to a sha256sum of the snapshot file itself in the source code as a belt-and-suspenders remediation for issues like this.

Definitely worth thinking about but seems not as urgent as the utxo set hash and its probably fine to release 26.0 without this since we don’t support mainnet use yet.

This is also a pretty wide design space as shown by the discussion in IRC today: https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2023-10-19#976439; (primarily posting this answer to save a link to this discussion)

My takeaway from tha discssion: It may be beneficial to wait and see what kind of hash commitment will be optimal for a potential future p2p distribution of the utxo set. And I guess we have at least time with this until we release 27.0 with mainnet chain params. @jamesob maybe we should have a brainstorming issue for p2p distribution of the utxo set for assumeutxo and move this conversation there?

fjahr commented at 7:31 pm on October 19, 2023: contributor

Actually #27669 is already there for this.

achow101 referenced this in commit da8e397e4a on Oct 23, 2023

achow101 closed this on Oct 23, 2023

Assumeutxo: Altered txoutset dump is still valid #28675