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).
It doesn't make sense to be checking whether the fee paid is underpaying
before we've finished setting the fees. So do that after we have done
the reduction for SFFO and change adjustment for fee overpayment.
d4b3483cec Primitives: Correct CTransaction deserialization docstring (TheCharlatan)
Pull request description:
Since https://github.com/bitcoin/bitcoin/pull/8589 CTxWitness was removed and instead replaced with CScriptWitness inside each CTxIn.
ACKs for top commit:
w0xlt:
ACK d4b3483
Tree-SHA512: 02bb73e8a7d1fc449e4776a162009261baecc573837fade74ad7d76b3cd63200424e02fd0abd000c63706072f2ab3c95d3053139495b81347463f43e56192ca9
Surprisingly easy to do. Almost all of the diff resolution was mechanically
* replacing boost::variant with std::variant
* replacing Optional with std::optional
* then replacing `nullopt` with `std::nullopt`
* updating the RPC functions for the new RPCArg::Default type
* update the tests/ directory to make new (since 22) tests use arrays for
createrawtransaction outputs
* other ad-hoc changes to function parameters etc (not too many of these)
I had to "really" change the code in PrecomputePSBTData, which was introduced
in 22.0 and affected by PSET, but this function was like 8 lines long so it
was easy.
Reviewing the diff may be a bit difficult because of the mix of mechanical
changes and ad-hoc things. Probably the most straightforward thing to do
is to redo the merge, `sed -i` to fix the boost::variant and Optional stuff,
then diff the remaining conflicts against this commit.
TODO: grep for `blindpsbt` and you will see that this RPC is still referenced
in documentation and help text even though it was deleted. Need to fix this
in 0.21 in a separate PR.
fade6195b1 Move TX_MAX_STANDARD_VERSION to policy (MarcoFalke)
Pull request description:
`primitives` should only be used for the raw datastructures (parsing and format). It is not the right place to document relay policy.
ACKs for top commit:
laanwj:
Code review ACK fade6195b1
lontivero:
Concept ACK https://github.com/bitcoin/bitcoin/pull/20611/commits/fade6195b1c230edd561443637a7bde81c2594a4
Tree-SHA512: f809c4aecd14d7e9feaa7b50b9c0697232991eef36190cd960bcfb0ad6e20c71a4f6aab48c7747cf8a681eb14feda60c55b09a37f128673d519567224f29cd97
This finally retires the old serialization framework, so I had to implement
the new serialization for all the Elements types. So there's a fair bit of
new code here but it's all straightforward rearrangements of the old code.
Was able to remove some const casts which was nice.
