2fd34ba504957331f5a08614b6e1f8317260f04d Add sanity checks for various ATMPArgs booleans (Greg Sanders)
20d8936d8bb6137a5a3722d34e0d7ae756031009 [refactor] make some members MemPoolAccept-wide (glozow)
cbbfe719b223b9e05398227cef68c99eb97670bd cpfp carveout is excluded in packages (glozow)
69f7ab05bafec1cf06fd7a58351f78e32bbfa2cf Add m_allow_sibling_eviction as separate ATMPArgs flag (Greg Sanders)
57ee3029ddadaee5cb48224e0a87f704b7971bd8 Add description for m_test_accept (Greg Sanders)
Pull request description:
First few commits of https://github.com/bitcoin/bitcoin/pull/28984 to set the stage for the package RBF logic.
These refactors are preparation for evaluating an RBF in a multi-proposed-transaction context instead of only a single proposed transaction. Also, carveouts and sibling evictions only should work in single RBF cases so add logic to preclude multi-tx cases in the future.
No behavior changes aside from bailing earlier from failed carve-outs.
ACKs for top commit:
glozow:
reACK 2fd34ba504957331f5a08614b6e1f8317260f04d via range-diff
sr-gi:
utACK [2fd34ba](2fd34ba504)
theStack:
re-ACK 2fd34ba504957331f5a08614b6e1f8317260f04d
Tree-SHA512: 5071c5b8d9b8d2c9faa278c8c4df31de288cb407a68e4d55544c588caff6c86160cce7825453549c6ed69e29d9ccb5ee2d4a518b18f563bfb12f2ced073fe42a
542e13b2937356810bda2c41be83c3b1675e2f2f rpc: Enhance metadata of the dumptxoutset output (Fabian Jahr)
4d8e5edbaa94805be41ae4c8aa2f4bf7aaa276fe assumeutxo: Add documentation on dumptxoutset serialization format (Fabian Jahr)
c14ed7f384075330361df636f40121cf25a066d6 assumeutxo: Add test for changed coin size value (Fabian Jahr)
de95953d870c41436de67d56c93259bc66fe1434 rpc: Optimize serialization disk space of dumptxoutset (Fabian Jahr)
Pull request description:
The second attempt at implementing the `dumptxoutset` space optimization as suggested in #25675. Closes#25675.
This builds on the work done in #26045, addresses open feedback, adds some further improvements (most importantly usage of compact size), documentation, and an additional test.
The [original snapshot at height 830,000](https://github.com/bitcoin/bitcoin/pull/29551) came in at 10.82 GB. With this change, the same snapshot is 8.94 GB, a reduction of 17.4%.
This also enhances the metadata of the output file and adds the following data to allow for better error handling and make future upgrades easier:
- A newly introduced utxo set magic
- A version number
- The network magic
- The block height
ACKs for top commit:
achow101:
ACK 542e13b2937356810bda2c41be83c3b1675e2f2f
TheCharlatan:
Re-ACK 542e13b2937356810bda2c41be83c3b1675e2f2f
theStack:
ACK 542e13b2937356810bda2c41be83c3b1675e2f2f
Tree-SHA512: 0825d30e5c3c364062db3c6cbca4e3c680e6e6d3e259fa70c0c2b2a7020f24a47406a623582040988d5c7745b08649c31110df4c10656aa25f3f27eb35843d99
84900ac34f6888b7a851d0a6a5885192155f865c doc: add release-notes-27064.md (Matthew Zipkin)
855dd8d592c951a2b3239867ffbf66bb8677d470 system: use %LOCALAPPDATA% as default datadir on windows (Matthew Zipkin)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/2391
This PR changes the default datadir location on Windows from `C:\Users\Username\AppData\Roaming\Bitcoin` to `C:\Users\Username\AppData\Local\Bitcoin`. This change only applies to fresh installs. To preserve backwards compatibility, on startup we check for the existence of `C:\Users\Username\AppData\Roaming\Bitcoin\chainstate` and if it is there, we continue using the "Roaming" directory as the default datadir location.
[Note that in Windows 11 this change may be moot:](https://learn.microsoft.com/en-us/uwp/api/windows.storage.applicationdata.roamingfolder?view=winrt-22621)
> Roaming data and settings is no longer supported as of Windows 11. The recommended replacement is [Azure App Service](https://learn.microsoft.com/en-us/azure/app-service/). Azure App Service is widely supported, well documented, reliable, and supports cross-platform/cross-ecosystem scenarios such as iOS, Android and web. Settings stored here no longer roam (as of Windows 11), but the settings store is still available.
ACKs for top commit:
achow101:
ACK 84900ac34f6888b7a851d0a6a5885192155f865c
BenWestgate:
crACK 84900ac34f
hebasto:
re-ACK 84900ac34f6888b7a851d0a6a5885192155f865c, only addressed feedback since my recent [review](https://github.com/bitcoin/bitcoin/pull/27064#pullrequestreview-2028718273).
Tree-SHA512: 807c6e89571287e2c8f4934229aec91ef28e7d0a675234acf1b7d085c24c7b73a08b6e345fbfc9038e6239187b6b69c08490ddaa1c057de5ea975c4a000bba42
No change in behavior.
For single transaction acceptance, this is a simple refactor:
Workspace::m_all_conflicting
Workspace::m_conflicting_fees
Workspace::m_conflicting_size
Workspace::m_replaced_transactions
are now grouped under a new SubPackageState struct that is
a member of MemPoolAccept.
And local variables m_total_vsize and m_total_modified_fees are now
SubpackageState members so they can be accessed from
PackageMempoolChecks.
We want these to be package-wide variables because
- Transactions could conflict with the same tx (just not the same
prevout), or their conflicts could share descendants.
- We want to compare conflicts with the package fee rather than
individual transaction fee.
We reset these MemPoolAccept-wide fields for each subpackage
evaluation to not cause state leaking, similar to temporary
coins.
The behavior is not new, but this rule exits earlier than before.
Previously, a carve out could have been granted in PreChecks() but then
nullified in PackageMempoolChecks() when CheckPackageLimits() is called
with the default limits.
154b2b2296edccb5ed24e829798dacb6195edc11 [fuzz] V3_MAX_VSIZE and effective ancestor/descendant size limits (glozow)
a29f1df289cf27c6cbd565448548b3dc1392a9b0 [policy] restrict all v3 transactions to 10kvB (glozow)
d578e2e3540e085942001350ff3aeb047bdac973 [policy] explicitly require non-v3 for CPFP carve out (glozow)
Pull request description:
Opening for discussion / conceptual review.
We like the idea of a smaller maximum transaction size because:
- It lowers potential replacement cost (i.e. harder to do Rule 3 pinning via gigantic transaction)
- They are easier to bin-pack in block template production
- They equate to a tighter memory limit in data structures that are bounded by a number of transactions (e.g. orphanage and vExtraTxnForCompact). For example, the current memory bounds for orphanage is 100KvB * 100 = 40MB, and guaranteeing 1 tx per peer would require reserving a pretty large space.
History for `MAX_STANDARD_TX_WEIGHT=100KvB` (copied from https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2115459510):
- 2010-09-13 In 3df62878c3 satoshi added a 100kB (MAX_BLOCK_SIZE_GEN/5 with MBS_GEN = MAX_BLOCK_SIZE/2) limit on new transactions in CreateTransaction()
- 2013-02-04 https://github.com/bitcoin/bitcoin/pull/2273 In gavin gave that constant a name, and made it apply to transaction relay as well
Lowering `MAX_STANDARD_TX_WEIGHT` for all txns is not being proposed, as there are existing apps/protocols that rely on large transactions. However, it's been brought up that we should consider this for TRUCs (which is especially designed to avoid Rule 3 pinning).
This reduction should be ok because using nVersion=3 isn't standard yet, so this wouldn't break somebody's existing use case. If we find that this is too small, we can always increase it later. Decreasing would be much more difficult.
~[Expected size of a commitment transaction](https://github.com/lightning/bolts/blob/master/03-transactions.md#expected-weight-of-the-commitment-transaction) is within (900 + 172 * 483 + 224) / 4 = 21050vB~ EDIT: this is incorrect, but perhaps not something that should affect how we choose this number.
ACKs for top commit:
sdaftuar:
ACK 154b2b2296edccb5ed24e829798dacb6195edc11
achow101:
ACK 154b2b2296edccb5ed24e829798dacb6195edc11
instagibbs:
ACK 154b2b2296edccb5ed24e829798dacb6195edc11
t-bast:
ACK 154b2b2296
murchandamus:
crACK 154b2b2296edccb5ed24e829798dacb6195edc11
Tree-SHA512: 89392a460908a8ea9f547d90e00f5181de0eaa9d2c4f2766140a91294ade3229b3d181833cad9afc93a0d0e8c4b96ee2f5aeda7c50ad7e6f3a8320b9e0c5ae97
d7707d9843b03f20d2a8c5a45d7b3db58e169e6f rpc: avoid copying into UniValue (Cory Fields)
Pull request description:
These are the simple (and hopefully obviously correct) copies that can be moves instead.
This is a follow-up from https://github.com/bitcoin/bitcoin/pull/30094#issuecomment-2108751842
As it turns out, there are hundreds of places where we copy UniValues needlessly. It should be the case that moves are always preferred over copies, so there should be no downside to these changes.
willcl-ark, however, noticed that memory usage may increase in some cases. Logically this makes no sense to me. The only plausible explanation imo is that because the moves are faster, more ops/second occur in some cases.
This list of moves was obtained by changing the function signatures of the UniValue functions to accept only rvalues, then compiling and fixing them up one by one. There still exist many places where copies are being made. These can/should be fixed up, but weren't done here for the sake of doing the easy ones first.
I ran these changes through clang-tidy with `performance-move-const-arg` and `bugprone-use-after-move` and no bugs were detected (though that's obviously not to say it can be trusted 100%).
As stated above, there are still lots of other less trivial fixups to do after these including:
- Using non-const UniValues where possible so that moves can happen
- Refactoring code in order to be able to move a UniValue without introducing a use-after-move
- Refactoring functions to accept UniValues by value rather than by const reference
ACKs for top commit:
achow101:
ACK d7707d9843b03f20d2a8c5a45d7b3db58e169e6f
ryanofsky:
Code review ACK d7707d9843b03f20d2a8c5a45d7b3db58e169e6f. No changes since last review other than rebase. I agree benchmarks showing increased peak memory usage and RSS are surprising, but number of allocations is down as expected, and runtime is also decreased.
willcl-ark:
ACK d7707d9843b03f20d2a8c5a45d7b3db58e169e6f
Tree-SHA512: 7f511be73984553c278186286a7d161a34b2574c7f5f1a0edc87c2913b4c025a0af5241ef9af2df17547f2e4ef79710aa5bbb762fc9472435781c0488dba3435
6629d1d0f8285d1bf2d87341a856abe903f26c13 test: improve robustness of connect_nodes() (furszy)
Pull request description:
Decoupled from #27837 because this can help other too, found it investigating a CI failure https://cirrus-ci.com/task/5805115213348864?logs=ci#L3200.
The `connect_nodes` function in the test framework relies on a stable number of peer
connections to verify that the new connection between the nodes is successfully established.
This approach is fragile, as any of the peers involved in the process can drop, lose, or
create a connection at any step, causing subsequent `wait_until` checks to stall indefinitely
even when the peers in question were connected successfully.
This commit improves the situation by using the nodes' subversion and the connection
direction (inbound/outbound) to identify the exact peer connection and perform the
checks exclusively on it.
ACKs for top commit:
stratospher:
reACK 6629d1d.
achow101:
ACK 6629d1d0f8285d1bf2d87341a856abe903f26c13
maflcko:
utACK 6629d1d0f8285d1bf2d87341a856abe903f26c13
AngusP:
re-ACK 6629d1d0f8285d1bf2d87341a856abe903f26c13
Tree-SHA512: 5f345c0ce49ea81b643e97c5cffd133e182838752c27592fcdeac14ad10919fb4b7ff38e289e42a7c3c638a170bd0d0b7a9cd493898997a2082a7b7ceef4aeeb
9f4ff1e9659597307f62510f1885ad8da3a1d9a3 windeploy: Renew certificate (Ava Chow)
Pull request description:
Renewed the Windows code signing certificate for another 3 years.
ACKs for top commit:
fanquake:
ACK 9f4ff1e9659597307f62510f1885ad8da3a1d9a3
glozow:
tested ACK 9f4ff1e96595
Tree-SHA512: 6441aa55fa1aa4e532bbe1ce1ad8b9be8a197d8de0aa57a8b02d41681a433599f305eb0a519e90782cebe20f473322be519ff6398933f525c39cc3476e860283
1e54d61c4698debf3329d1960e06078ccbf8063c test: add coverage for `mapped_as` from `getrawaddrman` (brunoerg)
8c2714907d1e1ffc58487b3b43e018c1ec10065b net: rpc: return peer's mapped AS in getrawaddrman (brunoerg)
Pull request description:
This PR adds two new fields in `getrawaddrman` RPC: "mapped_as" and "source_mapped_as". These fields are used to return the ASN (Autonomous System Number) mapped to the peer and its source. With these informations we can have a better view of the bucketing logic with ASMap specially in projects like [addrman-observer](https://github.com/0xb10c/addrman-observer).
ACKs for top commit:
fjahr:
Code review ACK 1e54d61c4698debf3329d1960e06078ccbf8063c
virtu:
ACK [1e54d61](1e54d61c46)
0xB10C:
ACK 1e54d61c4698debf3329d1960e06078ccbf8063c
glozow:
ACK 1e54d61c4698debf3329d1960e06078ccbf8063c
Tree-SHA512: af86bcc7a2e69bebd3fa9eaa2e527e0758c44c0a958de7292514d5f99f8f01f5df3bae11400451268e0255f738ff3acccc77f48fe129937512f1e9d9963c4c5e
21b8a14d37c19ce292d5529597e0d52338db48a9 depends: Fetch miniupnpc sources from an alternative website (Hennadii Stepanov)
Pull request description:
The https://miniupnp.tuxfamily.org website is unavailable now.
ACKs for top commit:
achow101:
ACK 21b8a14d37c19ce292d5529597e0d52338db48a9
edilmedeiros:
utACK 21b8a14d37c19ce292d5529597e0d52338db48a9
theuni:
utACK 21b8a14d37c19ce292d5529597e0d52338db48a9
Tree-SHA512: 141427447d5f1902f17fe3774fadf0ac912f663d7fd6467dff42fd6131f6884906b77cf49bbe7387f118fd9e9d52af1d36778624f9f02237d2ba3387fc974be6
Test addresses are being mapped according to the ASMap
file provided properly. Compare the result of the `getrawaddrman`
RPC with the result from the ASMap Health Check.
fa73431dd4709754c34a4d5ad1c940ff9e628cf3 ci: Add mising -Wno-error=maybe-uninitialized to armhf task (MarcoFalke)
Pull request description:
This happens after bd597c33e3e58cd3c6b22ed42f8f1fd7ff886bb2 in many pull requests as a silent merge conflict. For example:
* https://github.com/bitcoin/bitcoin/pull/29720#issuecomment-2120847661
* https://github.com/bitcoin/bitcoin/pull/29521#issuecomment-2106542236
* (Probably many undetected, because the CI task was not yet re-run)
* ...
ACKs for top commit:
fjahr:
utACK fa73431dd4709754c34a4d5ad1c940ff9e628cf3
fanquake:
ACK fa73431dd4709754c34a4d5ad1c940ff9e628cf3 - many fixed with 13.x
Tree-SHA512: 6e6ff8dc6f3c6a2abcd04c4203d3468f6e98c1ad3a4da4ad0037a9ee2cbec6bec079a5f778aba0273e38e173849927abcdfcfba7643d08ed66c1168cb89fab08
Adjust the security check for:
ld64.lld: warning: Option `-allow_stack_execute' is not yet implemented.
ld64.lld: error: -fixup_chains is incompatible with -no_pie
and to account for the embedding of LLVMs version number.
a057869aa3c42457570765966cb66accb2375b13 build: pass --with-ecmult-gen-kb=86 to secp256k1 (fanquake)
ca3d945dc66e177e8fa3e83c77236de89cc0072a Squashed 'src/secp256k1/' changes from d8311688bd..06bff6dec8 (fanquake)
Pull request description:
This includes changes from the 0.5.0 release: https://github.com/bitcoin-core/secp256k1/releases/tag/v0.5.0
> New function secp256k1_ec_pubkey_sort that sorts public keys using lexicographic (of compressed serialization) order.
> The implementation of the point multiplication algorithm used for signing and public key generation was changed, resulting in improved performance for those operations.
> The related configure option --ecmult-gen-precision was replaced with --ecmult-gen-kb (ECMULT_GEN_KB for CMake).
> This changes the supported precomputed table sizes for these operations. The new supported sizes are 2 KiB, 22 KiB, or 86 KiB (while the old supported sizes were 32 KiB, 64 KiB, or 512 KiB).
ACKs for top commit:
hebasto:
ACK a057869aa3c42457570765966cb66accb2375b13, I've got a zero diff with my local branch, which reproduces the subtree update, and `ecmult gen table size = 86 KiB` in the configure summary.
jonasnick:
utACK a057869aa3c42457570765966cb66accb2375b13
Tree-SHA512: 907012b0d7e0a6bd68b245c238e968f2318d8ac5de5ec9070245de8391c996eb5ec6428184d028f6f0f54d3b2f5a8292ad7081177e1c331397879505436dc38e
b3efb486732f3caf8b8a8e9d744e6d20ae4255ef protocol: make message types constexpr (Vasil Dimov)
2fa9de06c2c8583ee8e2434dc97014b26e218ab5 net: make the list of known message types a compile time constant (Vasil Dimov)
Pull request description:
Turn the `std::vector` to `std::array` because it is cheaper and allows us to have the number of the messages as a compile time constant: `ALL_NET_MESSAGE_TYPES.size()` which can be used in future code to build other `std::array`s with that size.
---
This change is part of https://github.com/bitcoin/bitcoin/pull/29418 but it makes sense on its own and would be good to have it, regardless of the fate of https://github.com/bitcoin/bitcoin/pull/29418. Also, if this is merged, that would reduce the size of https://github.com/bitcoin/bitcoin/pull/29418, thus the current standalone PR.
ACKs for top commit:
achow101:
ACK b3efb486732f3caf8b8a8e9d744e6d20ae4255ef
jonatack:
ACK b3efb486732f3caf8b8a8e9d744e6d20ae4255ef
maflcko:
utACK b3efb486732f3caf8b8a8e9d744e6d20ae4255ef 🎊
willcl-ark:
ACK b3efb486732f3caf8b8a8e9d744e6d20ae4255ef
Tree-SHA512: 6d3860c138c64514ebab13d97ea67893e2d346dfac30a48c3d9bc769a1970407375ea4170afdb522411ced306a14a9af4eede99e964d1fb1ea3efff5d5eb57af
This carve out is intended to allow a second child under restricted
circumstances, but this topology is not allowed for v3 transactions.
As CPFP carve out does not explicitly require a second child to actually
exist, it has the effect of granting a free +10KvB descendant size limit
when a single child is enough to bust the descendant limit.
The 'connect_nodes' function in the test framework relies
on a stable number of peer connections to verify the new
connection between the nodes is successfully established.
This approach is fragile, as any of the peers involved in
the process can drop, lose, or create a connection at any
step, causing subsequent 'wait_until' checks to stall
indefinitely even when the peers in question are connected
successfully.
This commit improves the situation by using the nodes' subversion
and the connection direction (inbound/outbound) to identify the
exact peer connection and perform the checks exclusively on it.
The assumption in the commit 188ca75e5fe4837d16241446558c7566912f67b2
about the broken `thread_local` implementation in GCC was misguided
because the initial failure was not due to GCC, but a bug in the Wine
runtime, as evidenced, for example, here:
- https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=917307
- https://bugs.freedesktop.org/show_bug.cgi?id=108662
Consequently, it is safe to re-enable `thread_local` support for
MinGW-w64 builds.
d35ba1b3f16071b8fe9b36398ba15352dbf2a54d util: avoid using thread_local variable that has a destructor (Vasil Dimov)
Pull request description:
Store the thread name in a `thread_local` variable of type `char[]` instead of `std::string`. This avoids calling the destructor when the thread exits. This is a workaround for
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=278701
For type-safety, return `std::string` from
`util::ThreadGetInternalName()` instead of `char[]`.
As a side effect of this change, we no longer store a reference to a `thread_local` variable in `CLockLocation`. This was dangerous because if the thread quits while the reference still exists (in the global variable `lock_data`, see inside `GetLockData()`) then the reference will become dangling.
ACKs for top commit:
laanwj:
Code review ACK d35ba1b3f16071b8fe9b36398ba15352dbf2a54d
hebasto:
re-ACK d35ba1b3f16071b8fe9b36398ba15352dbf2a54d.
theuni:
utACK d35ba1b3f16071b8fe9b36398ba15352dbf2a54d
Tree-SHA512: a2a3bc4401654d6e99db5b9c46a7051855f5a26886142298662e681b78dd581ff4c6bebe42f649b8e1fb8a78d569c6117302db2cd6362e884a22f2a5839b7d43
