[26.x] Changes for rc3 #28872
[26.x] Changes for rc3 #28872
Conversation
The negative bound for script threads comes from the machine which generates the man pages, so may only be correct for that machine. Any other placeholder value will also be wrong for some machines. Fix this be removing the value. This also fixes help2man incorrectly bolding the value, as if it were a paramater. Closes bitcoin#28850. Github-Pull: bitcoin#28858 Rebased-From: d799ea2
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
Would suggest backporting #28155 as mentioned in #28155 (comment). Am rebasing and updating #28248 today, which fixes a number of bugs including the regression reported in https://github.com/bitcoin/bitcoin/pull/27213/files#r1291926369 and described in #28248 (comment). If that isn't fixed in v26.0, it may be a good idea to add a warning to the notable changes section of the release notes where the changed behavior is described, as the way nodes have until now been able to ensure connections to various networks, via addnode, is the behavior that is impacted. |
No strong opinion, but I think that backports at this stage should either fix a regression or be clear bugfixes. Other behavior changes are better left for the next release, because they could themselves change behavior in a buggy way, see also the previous comment #28248 (comment) |
|
Yes, the first suggested one in #28155 (comment), i.e. 94e8882, is a bug fix. Am updating #28248 to punt for later on two of the seven, in favor of the clearest ones and the regression. |
c9e9995 to
571e14f
Compare
571e14f to
ec90cd7
Compare
|
I see the release notes have been added since my feedback above. If the regression in v26.0 is left unfixed, it would be a good idea to add a warning to the "Notable Changes" section of the release notes where the changed behavior is described, as the way nodes have until now been able to ensure connections to various networks and particularly the ones with fewer peers, via addnode, is the behavior that is impacted. The regression was first reported in August in https://github.com/bitcoin/bitcoin/pull/27213/files#r1291926369. Edit: I extracted #28895 from #28248 for the regression. Its first commit can be backported and it has 4 ACKs at this time of writing. |
This changes the PoolAllocator to default the alignment to the given type. This makes the code simpler, and most importantly fixes a bug on ARM 32bit that caused OOM: The class CTxOut has a member CAmount which is an int64_t and on ARM 32bit int64_t are 8 byte aligned which is larger than the pointer alignment of 4 bytes. So for CCoinsMap to be able to use the pool, we need to use the alignment of the member instead of just alignof(void*). Github-Pull: bitcoin#28913 Rebased-From: ce881bf
If alignment of the PoolAllocator would be insufficient, then the test would fail. This also catches the issue with ARM 32bit, where int64_t is aligned to 8 bytes but void* is aligned to 4 bytes. The test adds a check to ensure the pool has allocated a minimum number of chunks Github-Pull: bitcoin#28913 Rebased-From: d5b4c0b
Github-Pull: bitcoin#28925 Rebased-From: a6cc059
Github-Pull: bitcoin#28925 Rebased-From: 710da28
|
Suggesting to backport #28905 as well to ensure CI pass. |
to allocate our limited outbound slots correctly, and to ensure addnode connections benefit from their intended protections. Our addnode logic usually connects the addnode peers before the automatic outbound logic does, but not always, as a connection race can occur. If an addnode peer disconnects us and if it was the only one from its network, there can be a race between reconnecting to it with the addnode thread, and it being picked as automatic network-specific outbound peer. Or our internet connection or router, or the addnode peer, could be temporarily offline, and then return online during the automatic outbound thread. Or we could add a new manual peer using the addnode RPC at that time. The race can be more apparent when our node doesn't know many peers, or with networks like cjdns that currently have few bitcoin peers. When an addnode peer is connected as an automatic outbound peer and is the only connection we have to a network, it can be protected by our new outbound eviction logic and persist in the "wrong role". Examples on mainnet using logging added in the same pull request: 2023-08-12T14:51:05.681743Z [opencon] [net.cpp:1949] [ThreadOpenConnections] [net:debug] Not making automatic network-specific outbound-full-relay connection to i2p peer selected for manual (addnode) connection: [geh...odq.b32.i2p]:0 2023-08-13T03:59:28.050853Z [opencon] [net.cpp:1949] [ThreadOpenConnections] [net:debug] Not making automatic block-relay-only connection to onion peer selected for manual (addnode) connection: kpg...aid.onion:8333 2023-08-13T16:21:26.979052Z [opencon] [net.cpp:1949] [ThreadOpenConnections] [net:debug] Not making automatic network-specific outbound-full-relay connection to cjdns peer selected for manual (addnode) connection: [fcc...8ce]:8333 2023-08-14T20:43:53.401271Z [opencon] [net.cpp:1949] [ThreadOpenConnections] [net:debug] Not making automatic network-specific outbound-full-relay connection to cjdns peer selected for manual (addnode) connection: [fc7...59e]:8333 2023-08-15T00:10:01.894147Z [opencon] [net.cpp:1949] [ThreadOpenConnections] [net:debug] Not making automatic feeler connection to i2p peer selected for manual (addnode) connection: geh...odq.b32.i2p:8333 Finally, there does not seem to be a reason to make block-relay or short-lived feeler connections to addnode peers, as the addnode logic will ensure we connect to them if they are up, within the addnode connection limit. Fix these issues by checking if the address is an addnode peer in our automatic outbound connection logic. Github-Pull: bitcoin#28895 Rebased-From: cc62716
This change is required to work with the new windows-2022 image version 20231115 properly. Github-Pull: bitcoin#28905 Rebased-From: 91d5bd8
ec90cd7 to
740a85c
Compare
740a85c to
f868852
Compare
The `vmull_p64` is a part of the Crypto extensions from the ACLE. They are optional extensions, so they get enabled with a `+crypto` for architecture flags. Github-Pull: bitcoin#28919 Rebased-From: 228d6a2
Few further changes are expected, so reintegrate the release-notes.
f868852 to
2f86d30
Compare
b1d350c doc: update release notes for 26.0 (fanquake) b0546bc doc: update manual pages for 26.0 (fanquake) 9ce1766 build: bump version to v26.0 final (fanquake) Pull request description: Final changes for 26.0. Assuming no further backports. rc3 was done in #28872. ACKs for top commit: achow101: ACK b1d350c hebasto: ACK b1d350c, I have reviewed the code and it looks OK. Tree-SHA512: 8b1bfa9e9d6c5ccf8305335eba503c02a76043b2752e2302da84cb574078889ddb761b9efd14ef97f68bbae154b00ac54f531e2e33eba6baf8d703aa98ef5175
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Currently backports:
-par=#28858Also includes changes for rc3, and reintegrating the release-notes.