16bd283b3a Reapply "test: p2p: check that connecting to ourself leads to disconnect" (Sebastian Falbesoner)
0dbcd4c148 net: prevent sending messages in `NetEventsInterface::InitializeNode` (Sebastian Falbesoner)
66673f1c13 net: fix race condition in self-connect detection (Sebastian Falbesoner)
Pull request description:
This PR fixes a recently discovered race condition in the self-connect detection (see #30362 and #30368).
Initiating an outbound network connection currently involves the following steps after the socket connection is established (see [`CConnman::OpenNetworkConnection`](bd5d1688b4/src/net.cpp (L2923-L2930)) method):
1. set up node state
2. queue VERSION message (both steps 1 and 2 happen in [`InitializeNode`](bd5d1688b4/src/net_processing.cpp (L1662-L1683)))
3. add new node to vector `m_nodes`
If we connect to ourself, it can happen that the sent VERSION message (step 2) is received and processed locally *before* the node object is added to the connection manager's `m_nodes` vector (step 3). In this case, the self-connect remains undiscovered, as the detection doesn't find the outbound peer in `m_nodes` yet (see `CConnman::CheckIncomingNonce`).
Fix this by swapping the order of 2. and 3., by taking the `PushNodeVersion` call out of `InitializeNode` and doing that in the `SendMessages` method instead, which is only called for `CNode` instances in `m_nodes`.
The temporarily reverted test introduced in #30362 is readded. Fixes#30368.
Thanks go to vasild, mzumsande and dergoegge for suggestions on how to fix this (see https://github.com/bitcoin/bitcoin/issues/30368#issuecomment-2200625017 ff. and https://github.com/bitcoin/bitcoin/pull/30394#discussion_r1668290789).
ACKs for top commit:
naiyoma:
tested ACK [https://github.com/bitcoin/bitcoin/pull/30394/commits/16bd283b3ad05daa41259a062aee0fc05b463fa6](https://github.com/bitcoin/bitcoin/pull/30394/commits/16bd283b3ad05daa41259a062aee0fc05b463fa6), built and tested locally, test passes successfully.
mzumsande:
ACK 16bd283b3a
tdb3:
ACK 16bd283b3a
glozow:
ACK 16bd283b3a
dergoegge:
ACK 16bd283b3a
Tree-SHA512: 5b8aced6cda8deb38d4cd3fe4980b8af505d37ffa0925afaa734c5d81efe9d490dc48a42e1d0d45dd2961c0e1172a3d5b6582ae9a2d642f2592a17fbdc184445
51f7668d31 addrman: change nid_type from int to int64_t (Martin Zumsande)
051ba3290e addrman, refactor: introduce user-defined type for internal nId (Martin Zumsande)
Pull request description:
With `nIdCount` being incremented for each addr received, an attacker could cause an overflow in the past, see https://bitcoincore.org/en/2024/07/31/disclose-addrman-int-overflow/
Even though that attack was made infeasible indirectly by addr rate-limiting (PR #22387), to be on the safe side and prevent any regressions change the `nId`s used internally to `int64_t`.
This is being done by first introducing a user-defined type for `nId`s in the first commit, and then updating it to `int64_t` (thanks sipa for help with this!).
Note that `nId` is only used internally, it is not part of the serialization, so `peers.dat` should not be affected by this.
I assume that the only reason this was not done in the past is to not draw attention to this previously undisclosed issue.
ACKs for top commit:
naumenkogs:
ACK 51f7668d31
stratospher:
ACK 51f7668d31. I think it's a good change to make the nId space large(64 bits) so that the nId values are distinct.
achow101:
ACK 51f7668d31
Tree-SHA512: 68d4b8b0269a01a9544bedfa7c1348ffde00a288537e4c8bf2b88372ac7d96c4566a44dd6b06285f2fcf31b4f9336761e3bca7253fbc20db5e0d04e887156224
If we self-advertised to an inbound peer with the address they gave us,
nTime was left default-initialized, so that our peer wouldn't relay it
any further along.
Github-Pull: #25314
Rebased-From: 99b9e5f3a9
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
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.
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.
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.
Compare the expected script error, parsed from JSON, with the actual
error that VerifyScript returns. If the JSON does not include an
expected error, then skip this check.
Take the existing function to parse script errors and extend it to parse
Elements errors. SCRIPT_ERR_ERROR_COUNT is not included because it is a
pseudo error.
