Too early NotifyWalletLoaded() call in CWallet::Create() results the
notification goes before DescriptorScriptPubKeyMans were created and
added to an encrypted wallet.
Co-authored-by: Andrew Chow <achow101-github@achow101.com>
Github-Pull: bitcoin/bitcoin#24711
Rebased-From: 0c12f0116c
If the user has unchecked "Allow incoming connections" in
`Settings->Options...->Network` then `fListen=false` is saved in
`~/.config/Bitcoin/Bitcoin-Qt.conf`. This flips `-listen` to `false`
during startup, but leaves `-listenonion` to `true`.
This flipping of `-listen` is done in `OptionsModel::Init()` after
`InitParameterInteraction()` has been executed which would have flipped
`-listenonion`, should it have seen `-listen` being `false`
(this is a difference between `bitcoind` and `bitcoin-qt`).
Fixes: https://github.com/bitcoin-core/gui/issues/567
Github-Pull: bitcoin-core/gui#568
Rebased-From: 7f90dc26c8
This was changed in #22818 from 0 to 1. Since it changes
BLOCK_OPT_WIT of the genesis block, older versions of bitcoin
core would not read regtest directories created with newer versions
without a reindex.
Github-Pull: bitcoin#24527
Rebased-From: 5ce3057
Just add tests. No changes to application behavior. Tests will be
updated in the next commit changing & improving current behavior.
Include a Qt test for GUI startup crash reported by Rspigler in
https://github.com/bitcoin/bitcoin/issues/24457 caused by GetArg
behavior that happens if settings.json contains an integer value for any
of the configuration options which GUI settings can currently clash with
(-dbcache, -par, -spendzeroconfchange, -signer, -upnp, -natpmp, -listen,
-server, -proxy, -proxy, -onion, -onion, -lang, and -prune).
Github-Pull: bitcoin/bitcoin#24498
Rebased-From: 84b0973e35
There is a crashing bug in psbt.h which works as follows. This occurs in
the psbt_deserialize_input fuzz test, which deserializes an input and
then tries to reserialize it. Here a PSET input may contain a mainchain
transaction, which is where our trouble is.
The process is as follows:
1. On line 901, we create a CTransactionRef, which is a newtype around
std::shared_ptr<CTransaction> which defaults to being null.
2. On line 903 we then call `UnserializeFromVector` to populate this,
where this is a helper function which attempts to read some number of
objects from a byte vector, i.e. a length-prefixed blob.
3. HOWEVER, `UnserializeFromVector` when given an empty vector, decides
that it has successfully deserialized zero elements, and returns.
4. Then, on line 904 we assign the CTransactionRef, which is a valid
std::shared_ptr whose internal pointer is NULL, to `m_peg_in_tx`,
which is a variant of monostate, Bitcoin::CTransactionRef, and
CTransactionRef. Its variant changes from the default monostate
to CTransactionRef.
5. Then, on line 419, we call `std::get_if<CTransactionRef>` on this
object, which returns a std::optional<CTransactionRef>. Because
`m_peg_in_tx` is in the `CTransactionRef` variant, this succeeds,
returning a true std::optional containing a valid std::shared_ptr
which contains a NULL pointer.
6. Then, on line 420, we call `if (peg_in_tx)`, which is true, because
we have a true std::optional. We then dereference it on line 423,
which is perfectly legal, to get our std::shared_ptr, and pass this
shared pointer to SerializeToVector.
7. SerializeToVector passes through like 6 layers of serialize.h
obfuscation and eventually dereferences the shared pointer, but
because it's NULL, this is a NULL pointer dereference, and we get a
crash.
There are two lessons here:
1. Don't use C++. As I say in acf709b3ab,
where I introduced some of the offending code here (but only by
replacing boost stuff with their STL equivalents; the bug existed
before I did this), "what a trainwreck of a language".
2. Don't use `UnserializeFromVector` and expect it to throw if the data
you're deserializing is malformed. If it's malformed in the sense of
being empty, it will "succeed" and silently do nothing.
I glanced at every other instance of UnserializeFromVector to check what
will happen when it's passed an empty string. I believe there are no
other cases where it will fail to initialize a NULL pointer, so I
believe that this will not cause other crashes. But I also believe that
the behavior in this case is almost always wrong and that we parse
malformed PSETs in crazy and incorrect ways all over the place.
If anybody has a problem with this, I encourage you to go review the
Bitcoin PSBT2 PRs, which this stuff is based on, which have been
languishing in rebase hell for the better part of a decade. Don't blame
the author for not writing perfect code in a hostile language with no
support.
After this PR I ran the fuzzer for 16 hours on a 192-thread machine (so
3072 CPU-hours) and didn't see any more crashes.
ubsan doesn't like assigning arbitrary uint8_t values to bool. It's easy
to avoid doing, so do it. (We do this specifically in PSET since that's
Elements-specific code, but the same issue is present in Bitcoin in the
Unserialize impl for bool in serialize.h. Upstream this is only used in
the wallet database, where it may be that non 0/1 values are impossible
(absent a corrupt wallet).
When we have an invalid program, use a zero CMR and create a valid
control block/taptweak. Otherwise we fail to hit the Simplicity logic at
all with bad programs.
#
# You are committing on CAMUS
#
# Please enter the commit message for your changes. Lines starting
# with '#' will be ignored, and an empty message aborts the commit.
#
# On branch 2024-12--simple-fuzz
# Changes to be committed:
# modified: src/test/fuzz/simplicity_tx.cpp
#
# Untracked files:
# reduced-corpus/
# run-fuzz-merge-dir-CPzq/
# run-fuzz-merge-dir-sscf/
#
The first fuzztest takes a Simplicity program and a transaction and
directly calls the Simplicity interpreter with some context cobbled
together from the transaction. It also tries messing with the budget
and computes AMRs to check that the AMR-check works, even though on
the blockchain AMRs are never used.
It also attempts mangling programs to directly fuzz the parser, type
inference and CMR checking.
THIS test, on the other hand, takes a transaction, looks for Simplicity
programs (or witnesses which look like Simplicity programs), computes
their CMRs to produce a correct corresponding scriptPubKey, creates
scriptchecks, and executes them. This should do an end-to-end coverage
of the whole Simplicity consensus logic, including all the new branches
in interpreter.cpp.
To produce seeds for this, I have a a local fuzz target which uses
rust-simplicity and rust-elements to produce programs, deep Taproot trees,
and transactions. I run this to get high coverage, then dump the
resulting complete transactions to disk, where they can be used as
seeds for this test.
This fuzz target takes its seeds in a simple and well-defined format: a
four-byte LE budget, then a transaction, Simplicity program and witness,
each prefixed by a four-byte LE length. The fuzz target extracts any
additional randomness it needs from the txid of the first input of the
transaction, since this data is not interpreted in any other way we
therefore won't confuse the fuzzer.
The reason for this design, rather than a more typical "just query the
fuzzer when you need stuff", is to make it possible to fairly easily
generate test vectors from sources other than this fuzz test. (For
example, I have an alternate target which uses Rust code to generate
well-formed Simplicity programs, which quickly gets high coverage at the
expense of being an unmaintainable mess.)
This commit includes a .c file with a small function to comute the AMR
of a program. This is needed to pass a correct AMR to the Simplicity
interpreter, to exercise all the AMR-checking paths. In practice this is
not really necessary; Elements passes NULL to disable these AMR checks.
* feeDelta tracked the delta (to be applied on top of the actual fee)
* m_modified_fee tracks the actual fee with the delta included
* Instead of passing in the new total delta to the Updater, pass in by
how much the total delta should be modified.
This is needed for the next commit, but makes sense on its own because
the same is done by UpdateDescendantState and UpdateAncestorState.
Cherry-pick of fa52cf8e11https://github.com/bitcoin/bitcoin/pull/23418 (1/2)
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).
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.
