Skip to content

[28.x] Further backports and rc2 #30827

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Sep 17, 2024
Merged

[28.x] Further backports and rc2 #30827

merged 10 commits into from
Sep 17, 2024

Conversation

achow101
Copy link
Member

@achow101 achow101 commented Sep 5, 2024 •
edited
Loading

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 5, 2024 •
edited
Loading

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, hebasto
Stale ACK furszy

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

@achow101 achow101 marked this pull request as draft September 5, 2024 22:30
maflcko
maflcko reviewed Sep 6, 2024
View reviewed changes
@achow101 achow101 mentioned this pull request Sep 6, 2024
Merged
hebasto
hebasto approved these changes Sep 6, 2024
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK 37a6739.

@achow101 achow101 added this to the 28.0 milestone Sep 10, 2024
@furszy @achow101
assumeUTXO: fix peers disconnection during sync
c6b5db1 
Because AssumeUTXO nodes prioritize tip synchronization, they relay their local
address through the network before completing the background chain sync.
This, combined with the advertising of full-node service (NODE_NETWORK), can
result in an honest peer in IBD connecting to the AssumeUTXO node (while syncing)
and requesting an historical block the node does not have. This behavior leads to
an abrupt disconnection due to perceived unresponsiveness (lack of response)
from the AssumeUTXO node.

This lack of response occurs because nodes ignore getdata requests when they do
not have the block data available (further discussion can be found in PR 30385).

Fix this by refraining from signaling full-node service support while the
background chain is being synced. During this period, the node will only
signal 'NODE_NETWORK_LIMITED' support. Then, full-node ('NODE_NETWORK')
support will be re-enabled once the background chain sync is completed.

Github-Pull: bitcoin#30807
Rebased-From: 6d5812e
@furszy @achow101
test: add coverage for assumeUTXO honest peers disconnection
b329ed7 
Exercising and verifying the following points:

1. An IBD node can sync headers from an AssumeUTXO node at
   any time.

2. IBD nodes do not request historical blocks from AssumeUTXO
   nodes while they are syncing the background-chain.

3. The assumeUTXO node dynamically adjusts the network services
   it offers according to its state.

4. IBD nodes can fully sync from AssumeUTXO nodes after they
   finish the background-chain sync.

Github-Pull: bitcoin#30807
Rebased-From: 992f83b
@achow101 achow101 marked this pull request as ready for review September 11, 2024 17:43
@achow101 achow101 mentioned this pull request Sep 11, 2024
Merged
furszy
furszy reviewed Sep 11, 2024
View reviewed changes
Copy link
Member

@furszy furszy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

utACK b97323c

@DrahtBot DrahtBot requested a review from hebasto September 11, 2024 19:26
@achow101 achow101 mentioned this pull request Sep 12, 2024
Merged
@hebasto
Copy link
Member

hebasto commented Sep 13, 2024

@achow101

Could you please add bitcoin-core/gui#835 as well?

@furszy @achow101
gui: fix crash when closing wallet
674dded 
The crash occurs because 'WalletController::removeAndDeleteWallet' is called
twice for the same wallet model: first in the GUI's button connected function
'WalletController::closeWallet', and then again when the backend emits the
'WalletModel::unload' signal.

This causes the issue because 'removeAndDeleteWallet' inlines an
erase(std::remove()). So, if 'std::remove' returns an iterator to the end
(indicating the element wasn't found because it was already erased), the
subsequent call to 'erase' leads to an undefined behavior.

Github-Pull: bitcoin-core/gui#835
Rebased-From: a965f2b
@achow101
Copy link
Member Author

@achow101

Could you please add bitcoin-core/gui#835 as well?

Done

@hebasto
Copy link
Member

hebasto commented Sep 13, 2024

@achow101

Feel free to pick this translation update, which resolves #30897 and includes new translations made over the nearly 3 weeks since #30715.

@fanquake
Copy link
Member

Feel free to pick this translation update

There's not so much of a rush here that this should bypass the normal PR & review process. This should first be PR'd and merged into master and then backported. Otherwise any broken string are still in master. There is at least one other change waiting for 28.x, so rc2 also needs to either wait for that, or we'll be having an rc3 (which can include the ts update) in any case.

@hebasto
Copy link
Member

hebasto commented Sep 13, 2024 •
edited
Loading

There's not so much of a rush here that this should bypass the normal PR & review process.

Sure.

This should first be PR'd and merged into master and then backported.

After branching off, the Transifex.com has translations for the version branches only. Fetching 28x translations into the master branch does not look reasonable.

Otherwise any broken string are still in master.

Maybe this case is a special one.

@hebasto
Copy link
Member

hebasto commented Sep 13, 2024

There's not so much of a rush here that this should bypass the normal PR & review process.

Sure.

Please see #30899.

@hebasto hebasto mentioned this pull request Sep 13, 2024
Merged
fanquake added a commit that referenced this pull request Sep 16, 2024
@fanquake
Merge #30899: qt: Translations update
37679b8 
ae05295 qt: Translations update (Hennadii Stepanov)

Pull request description:

  The recent translations from Transifex.com fetched with the bitcoin-maintainer-tools/update-translations.py tool.

  Fixes #30897.

  Translations are updated, while skipping removing translations for 2 languages.

  Related:
  - #30827 (comment)

ACKs for top commit:
  achow101:
    ACK ae05295
  pablomartin4btc:
    ACK ae05295

Tree-SHA512: 52107dfccaaaef187389ecb114eef4283ff40a3b855de811685fc2d6cfb1d869b2610edf86b25a235266fd7dcb36f693a6816a60639246b5d439c4f6482b2ebd
@hebasto @achow101
qt: Translations update
1b853fd 
The recent translations from Transifex.com 28.x fetched with the
bitcoin-maintainer-tools/update-translations.py tool.

Github-Pull: bitcoin#30899
Rebased-From: ae05295
@achow101 achow101 marked this pull request as draft September 16, 2024 16:20
sipa and others added 4 commits September 16, 2024 23:10
@achow101 achow101 marked this pull request as ready for review September 17, 2024 03:10
@achow101 achow101 mentioned this pull request Sep 17, 2024
Merged
stickies-v
stickies-v approved these changes Sep 17, 2024
View reviewed changes
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 06a7df7

I verified that all backports are clean, except:

  • b329ed7 backported from 992f83b: merge conflict due to a difference in sha256sum_file import, trivially resolved

I didn't review each commit in-depth since they're clean, but the changes backported seem sensible.

@DrahtBot DrahtBot requested a review from furszy September 17, 2024 11:26
hebasto
hebasto approved these changes Sep 17, 2024
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment •
edited
Loading

Choose a reason for hiding this comment

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

ACK 06a7df7, I've backported the listed PRs locally. The only merge conflict I faced was in #30807. It was trivial to resolve.

@fanquake fanquake merged commit 1147e72 into bitcoin:28.x Sep 17, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fanquake fanquake fanquake left review comments

@hebasto hebasto hebasto approved these changes

@furszy furszy Awaiting requested review from furszy

+2 more reviewers

@maflcko maflcko maflcko left review comments

@stickies-v stickies-v stickies-v approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Backport
Projects
None yet
Milestone
28.0
Development

Successfully merging this pull request may close these issues.
9 participants
@achow101 @DrahtBot @hebasto @fanquake @furszy @maflcko @stickies-v @fjahr @sipa