elements/src/primitives
Andrew Poelstra a3b7e71a3d
transaction: refuse to deserialize TxOuts with null values
In Bitcoin Core the notion of a "null" amount (and therefore a "null"
txout) is one which cannot be serialized or deserialized. In Core this
is implemented using a value of -1.

In Elements we use the CT notion of "nullness" which is that the flag on
the confidential value is 0. See d53479c9ff
which implemented this. This is reasonable, but because we don't have
checks on deserialization, we can deserialize objects that cannot be
reserialized.

In particular, in coins.h, we deserialize a coin by deserializing its
txout. When reserializing we assert that !out.IsNull(). This assertion
is hit by the `coins_deserialize` fuzztest.

There are a few potential fixes here:

* Remove the assertion from coins.h, which is there to catch logic bugs
  in Core, on the assumption that if they have no bugs then we don't
  either. This seems like a bad idea.
* Change "nullness" for amounts to be an encoding of -1, like in Core.
  This seems dangerous because we call `GetAmount` all over the place,
  and if this could return the -1 amount, this will likely blow
  something up. Probably this is safe for the same reason it is in Core
  -- that is, we never create null txouts except as sentinel values. But
  do you wanna bet that this is true now? That it'll always be true?
* Same as above, but assert that the amount is not null. This is safer
  than just blindly hoping that no overflows will occur but still not
  obviously safe.
* Refuse to deserialize null CT objects. This is impossible because we
  use null nonce values in txouts.
* Refuse to deserialize null CT values. Similarly, this is impossible
  because we use null values in null asset issuances, which are legal.
* Refused to deserialize CTxOuts with null values or assets.

We are going with the latter solution, because it is narrowly scoped,
does not increase the "crash surface" (we throw an exception, and we
already throw exceptions for other kinds of invalid serializations),
and is very unlikely to cause bugs (null values are invalid on the
network anyway; this is the first check in VerifyAmounts) (so are null
assets for that matter, which we maybe also should refuse to
deserialize).
2025-02-06 21:31:47 +00:00
..
bitcoin Merge c53e95f22c into merged_master (Bitcoin PR bitcoin/bitcoin#23137) 2023-05-14 07:06:13 +00:00
block.cpp Merge 99813a9745 into merged_master (Bitcoin PR #17829) 2020-11-14 16:52:02 +00:00
block.h Merge 4a540683ec into merged_master (Bitcoin PR #20813) 2021-06-16 14:11:07 +00:00
confidential.cpp Add ConfidentialCommitment-based types 2019-03-20 12:05:40 +00:00
confidential.h transaction: refuse to deserialize TxOuts with null values 2025-02-06 21:31:47 +00:00
pak.cpp lint: fix non-pylint linter errors 2021-10-15 00:20:20 +00:00
pak.h lint: fix non-pylint linter errors 2021-10-15 00:20:20 +00:00
transaction.cpp Merge 623745ca74 into merged_master (Bitcoin PR bitcoin/bitcoin#23912) 2023-06-14 15:44:11 +00:00
transaction.h transaction: refuse to deserialize TxOuts with null values 2025-02-06 21:31:47 +00:00
txwitness.cpp [BROKEN] Introduce CA in data structures 2019-03-20 12:05:45 +00:00
txwitness.h Linter fixes 2023-08-31 14:47:07 -07:00