This originates in 8723debb3d which has no
PR associated with it. We've really gotta stop putting thousands of
unreviewed commits into this project and rebasing the history away..
We have an unsigned constant which we're bit-inverting, or'ing into a
signed constant, then assigning back to the signed constant. We should
make it explicit wth is going on here.
See: c0a50ce33e
The return value of 2 now indicates:
"A valid connected IGD has been found but its IP address is reserved (non routable)"
We continue to ignore any return value other than 1.
(cherry picked from commit 8acdf66540)
We cannot fuzz RBF (or do anything mempool-related, really) without
chainparams. This has been true since 2019 at least. I suspect this fuzz
test has never really been run.
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).
While the bitcoin/elements project testing isn't trying to gain coverage of the regular
libsecp256k1 library, in our case we do want our (fuzz) testing to cover
simplicity's own copy of libsecp256k1, which has been specifically trimmed down
to only include functionality needed by simplicity's jets.
The main difference is that there is now explicit simplicity deallocation functions to go with the allocation functions in the API.
There are some minor changes to error message text.
The deserialization code is now automatically generated.
The are some other minor internal changes.
Without this patch, the --with-sanitizers config flag has no effect on
the copy of libsimplicity in Elements Core. This means that we aren't
running asan or tsan when we intend to, and also means that when fuzzing
we aren't instrumenting the Simplicity binary.
The result is extremely bad fuzz coverage and missed bugs.
ubsan suppression for simplicity sha256.c
ubsan detects when a left shift would overflow an integer type. This is
not UB (it would be if you tried to shift more than the type's width in
one shot) but "may be unintentional" and is therefore detected.
Add a whitelist to the giant list of whitelists.
We have the code fragment `txTo.GetHash().begin()`, which takes a
transaction, computes its txid as a uint256, and then saves a pointer to
the internal data of the uint256.
However, in C++, expressions of the form a.b().c() lead to the return
value of `b` being dropped immediately after the call to `c`. This is
fine if `c` is something like `GetHex` which returns a new independently
allocated object with no pointers to its input. It is not fine for
`begin` which returns a pointer into the return value of `GetHash`.
So this fragment returns a dangling pointer, which is later used by the
Simplicity interpreter, leading to UB.
In practice this code appeared to work, possibly because the stack
layout was such that it actually did work ok. Or possibly because we
don't test with enough fidelity to tell that Simplicity's view of the
txid of a transaction was mangled.
