Skip to content

Prepare 23.2.6 final #1394

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 22 commits into from
Feb 9, 2025
Merged

Conversation

psgreco
Copy link
Contributor

@psgreco psgreco commented Feb 9, 2025

Includes the version bump and all the fixes from master up to 29cf7c2 but with simplicity reverted, to keep 23.2.x without it (only test fixes)

apoelstra and others added 22 commits January 31, 2025 11:39
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.
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.
c2d4c3d07b Update elements-sources.mk
5ba4a709ac Add explicit deallocation functions to the Simplicity API
fd3a1a7c3f Fix comments
e10d34fa49 Clarify comment about illegal hidden children
3b55e8150e Rename STOP code error message
f67d1ad145 Generate contents of decodePrimitive automatically
991fddcb6a Correctly name identity hashes
a840706c2f Static asserts on imr_buf not needed
7e91641218 Prep C code for VST proving

git-subtree-dir: src/simplicity
git-subtree-split: c2d4c3d07bb7e5973fc22cf172ed8fb9a84b4365
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.
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.
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 d53479c
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).
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.
…fuzz-fixes

A couple miscellaneous fixes for fuzztests
…onal

Fix some functional tests when bdb is not available
…simplicity"

This reverts commit 368010a, reversing
changes made to e9a9144.
…2--misc-fuzz-fixes"

This reverts commit 72a5e41, reversing
changes made to 368010a.
@psgreco psgreco requested a review from delta1 February 9, 2025 05:05
@jsarenik
Copy link

jsarenik commented Feb 9, 2025

What about four failing CI tasks?

Screenshot_20250209-090629

@delta1
Copy link
Member

delta1 commented Feb 9, 2025

@jsarenik those are unrelated to this PR and mostly fixed by #1393

utACK 320b49d

@delta1 delta1 merged commit c3e25d7 into ElementsProject:elements-23.2.x Feb 9, 2025
9 of 13 checks passed
@psgreco psgreco deleted the elem-23.2.6-final branch February 9, 2025 14:27
@psgreco
Copy link
Contributor Author

psgreco commented Feb 9, 2025

@jsarenik a bit more detail on what Byron explained

TSAN fails due to a change in the runners, clang-13 doesn't like vm.mmap_rnd_bits=32 that's set in the runners, works locally without issues. #1393 will upgrade clang to 18, which has the bug fixed.
CentOS 8 fails due to the release being pulled from the repos, so the updates fail. Moving it to Rocky 8 will keep the same environment and make it work again.

The other 2 failing tests are related to native builds, which are not used for releases, they are both for testing debug environments. Still we're working on making them work again, but nothing that affects the release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants