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

[24.x] Backports for rc2 #26133

Merged
merged 15 commits into from Oct 13, 2022
Merged

[24.x] Backports for rc2 #26133

merged 15 commits into from Oct 13, 2022

Conversation

@instagibbs
Copy link
Member

anymore backports expected for rc2?

@maflcko
Copy link
Member

maflcko commented Oct 7, 2022

achow101 and others added 5 commits October 11, 2022 09:19
Follow-up to bitcoin#25717. The commit "Utilize anti-DoS headers download
strategy" changed how this bool variable is computed, so that its value
is now the opposite of what it should be.

GitHub-Pull: bitcoin#26172
Rebased-From: bdcafb9
Our required Python version 3.6.12 does not support `capture_output` as
a subprocess.run argument; this was added in python 3.7.

We can emulate it by setting stdout and stderr to subprocess.PIPE

Github-Pull: bitcoin#26212
Rebased-From: be59bd1
Since commit f08c9fb from PR
bitcoin#21726, index
`BlockUntilSyncedToCurrentChain` behavior has been less reliable, and there has
also been a race condition in the `coinstatsindex_initial_sync` unit test.

It seems better for `BlockUntilSyncedToCurrentChain` to actually wait for the
last connected block to be fully processed, than to be able to return before
prune locks are set, so this switches the order of `m_best_block_index =
block;` and `UpdatePruneLock` statements in `SetBestBlockIndex` to make it more
reliable.

Also since commit f08c9fb, there has been a
race condition in the `coinstatsindex_initial_sync` test. Before that commit,
the atomic index best block pointer `m_best_block_index` was updated as the
last step of `BaseIndex::BlockConnected`, so `BlockUntilSyncedToCurrentChain`
could safely be used in tests to wait for the last `BlockConnected`
notification to be finished before stopping and destroying the index. But
after that commit, calling `BlockUntilSyncedToCurrentChain` is no longer
sufficient, and there is a race between the test shutdown code which destroys
the index object and the new code introduced in that commit calling
`AllowPrune()` and `GetName()` on the index object. Reproducibility
instructions for this are in
bitcoin#25365 (comment)

This commit fixes the `coinstatsindex_initial_sync` race condition, even though
it will require an additional change to silence TSAN false positives,
bitcoin#26188, after it is fixed. So this
partially addresses but does not resolve the bug reporting TSAN errors
bitcoin#25365.

There is no known race condition outside of test code currently, because the
bitcoind `Shutdown` function calls `FlushBackgroundCallbacks` not
`BlockUntilSyncedToCurrentChain` to safely shut down.

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
Co-authored-by: MacroFake <falke.marco@gmail.com>

Github-Pull: bitcoin#26215
Rebased-From: 8891949
@dergoegge
Copy link
Member

ACK 5ad82a0

stickies-v and others added 9 commits October 13, 2022 23:36
ReacceptWalletTransactions is replaced by ResubmitWalletTransactions
which already handles acquiring the necessary locks internally.

Github-Pull: bitcoin#26205
Rebased-From: 01f3534
Moves the logic of whether or not transactions should actually be
resent out of the function that's resending them. This reduces
responsibilities of ResubmitWalletTransactions and allows
carving out the updating of m_next_resend in a future commit.

Github-Pull: bitcoin#26205
Rebased-From: 7fbde8a
We only want to relay our resubmitted transactions once every 12-36h.
By separating the timer update logic out of ResubmitWalletTransactions
and into MaybeResendWalletTxs we avoid non-relay calls (previously in
the separate ReacceptWalletTransactions function) from resetting that
timer.

Github-Pull: bitcoin#26205
Rebased-From: 9245f45
Since m_next_resend is now only called from MaybeResendWalletTxs()
we don't have any potential race conditions anymore, so the usage
of std::atomic can be reverted.

Github-Pull: bitcoin#26205
Rebased-From: b01682a
Merging should be checking that the current PSBTOutput doesn't have a
taptree and the other one's is copied over. The original merging had
this inverted and would remove m_tap_tree if the other did not have it.

Github-Pull: bitcoin#25858
Rebased-From: 7df6e1b
Instead of having an entire TaprootBuilder which may or may not be
complete, and could potentially have future changes that interact oddly
with taproot tree tuples, have m_tap_tree be just the tuples.

When needed in other a TaprootBuilder for actual use, the tuples will be
added to a a TaprootBuilder that, in the future, can take in whatever
other data is needed as well.

Github-Pull: bitcoin#25858
Rebased-From: 0577d42
@dergoegge
Copy link
Member

ACK e2e4c29

1 similar comment
@achow101
Copy link
Member

ACK e2e4c29

@instagibbs
Copy link
Member

ACK e2e4c29

@achow101 achow101 merged commit 885366c into bitcoin:24.x Oct 13, 2022
@fanquake fanquake deleted the 24.0rc2_backports branch October 14, 2022 01:14
@LarryRuane
Copy link
Contributor

post-merge ACK e2e4c29

@bitcoin bitcoin locked and limited conversation to collaborators Oct 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet