Skip to content
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

[27.x] More backports #30467

Merged
merged 8 commits into from
Jul 24, 2024
Merged

[27.x] More backports #30467

merged 8 commits into from
Jul 24, 2024

Conversation

achow101 and others added 7 commits July 17, 2024 11:27
A common issue that our fuzzers keep finding is that outpoints don't
exist in the non witness utxos. Instead of trying to track this down and
checking in various individual places, do the check early during
deserialization.

Github-Pull: bitcoin#29855
Rebased-From: 9e13ccc
Initiating an outbound network connection currently involves the
following steps after the socket connection is established (see
 `CConnman::OpenNetworkConnection` method):
    1. set up node state
    2. queue VERSION message
    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`.

Thanks go to vasild, mzumsande, dergoegge and sipa for suggestions on
how to fix this.

Github-Pull: bitcoin#30394
Rebased-From: 66673f1
Now that the queueing of the VERSION messages has been moved out of
`InitializeNode`, there is no need to pass a mutable `CNode` reference any
more. With a const reference, trying to send messages in this method would
lead to a compile-time error, e.g.:

----------------------------------------------------------------------------------------------------------------------------------
...
net_processing.cpp: In member function ‘virtual void {anonymous}::PeerManagerImpl::InitializeNode(const CNode&, ServiceFlags)’:
net_processing.cpp:1683:21: error: binding reference of type ‘CNode&’ to ‘const CNode’ discards qualifiers
 1683 |     PushNodeVersion(node, *peer);
...
----------------------------------------------------------------------------------------------------------------------------------

Github-Pull: bitcoin#30394
Rebased-From: 0dbcd4c
…ect"

This reverts commit 9ec2c53 with
a tiny change included (identation of the wait_until call).

Github-Pull: bitcoin#30394
Rebased-From: 16bd283
This avoids situations during a reindex in which shutdown
doesn't finish since SyncWithValidationInterfaceQueue is
called by the load block thread when the scheduler is already stopped.

Github-Pull: bitcoin#30435
Rebased-From: 5fd4836
Fix cases of calls to `FillPSBT` returning `complete=true` when it's not
the case.

This can happen when some inputs have been signed but the transaction is
subsequently modified, e.g. in the context of PayJoins.

Also fixes a related bug where a finalized hex string is attempted to be
added during `walletprocesspsbt` but a CHECK_NONFATAL causes an abort.

Reported in bitcoin#30077.

Github-Pull: bitcoin#30357
Rebased-From: 39cea21
This test checks that we can successfully process PSBTs and opt out of
finalization.

Previously trying to call `walletprocesspsbt` would attempt to
auto-finalize (as a convenience), and would not permit opt-out of
finalization, instead aborting via `CHECK_NONFATAL`.

Github-Pull: bitcoin#30357
Rebased-From: 7e36dca
@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 17, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK stickies-v, willcl-ark

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 885c4b7 but #29855 is missing from release-notes.md

I verified that all backport commits are clean, except:

  • ab42206 backported from 16bd283: the relevant part of the test has been minimally carved out and added to 27.x which I think is a good approach

@fanquake fanquake force-pushed the 27_more_backports branch from 885c4b7 to 4f23c86 Compare July 23, 2024 14:47
@fanquake
Copy link
Member Author

but #29855 is missing from release-notes.md

Thanks, added.

@stickies-v
Copy link
Contributor

ACK 4f23c86

Copy link
Member

@willcl-ark willcl-ark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 4f23c86

Checked all commits were backported cleanly and had a release note for their original PR.

I agree with stickies that carving out the relevant portion of the test for ab42206 makes best sense for a backport.

@fanquake fanquake merged commit 0cbdc6b into bitcoin:27.x Jul 24, 2024
16 checks passed
@fanquake fanquake deleted the 27_more_backports branch July 24, 2024 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants