Prepare 23.2.6 final #1394
Merged
Prepare 23.2.6 final #1394
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
Update to latest Simplicity
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.
… descriptor wallets
…ble for descriptor wallets
… descriptor wallets
…isable for descriptor wallets
…fuzz-fixes A couple miscellaneous fixes for fuzztests
…onal Fix some functional tests when bdb is not available
|
What about four failing CI tasks?
|
delta1
approved these changes
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 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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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)