fa6d4891c7815025ab1cd58d0906466af31bb648 refactor: Use type-safe time in txorphanage (MarcoFalke)
Pull request description:
This allows to remove manual conversions like multiplication by `60`, and uses a type-safe type instead of a raw `int64_t`.
ACKs for top commit:
epiccurious:
utACK fa6d4891c7815025ab1cd58d0906466af31bb648.
dergoegge:
Code review ACK fa6d4891c7815025ab1cd58d0906466af31bb648
brunoerg:
utACK fa6d4891c7815025ab1cd58d0906466af31bb648
Tree-SHA512: c187d0e579b1131afcef8c901f5662c18ab867fa2a99fbb13b67bb1e10b2047128194bfef8329cde0d51e1c35d6227ae292b823968f37ea9422975e46e01846a
fac72985292b516919a216d9a78cf84418cd7f96 fuzz: Fix wallet_bdb_parser stdlib error matching (MarcoFalke)
Pull request description:
The stdlib error string is an implementation detail and can not be relied upon.
Ref: `libc++abi: terminating due to uncaught exception of type std::runtime_error: AutoFile::read: end of file: unspecified iostream_category error`
ACKs for top commit:
achow101:
ACK fac72985292b516919a216d9a78cf84418cd7f96
Tree-SHA512: 588acc71a05d97855d6bb65380411e8486692536434eadee7697de09f80b128ff2f90a31fd0e8384d084b554d2f3978efd076082e070e721cf05b07c94cc83b1
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
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
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.
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
d51fbab4b32d56765e8faab6ad01245fb259b0ca wallet, test: Be able to always swap BDB endianness (Ava Chow)
0b753156ce60c29efb2386954ba7555ad8f642f5 test: Test bdb_ro dump of wallet without reset LSNs (Ava Chow)
c1984f128284589423b7e0cc06c9a3b23a242d95 test: Test dumping dbs with overflow pages (Ava Chow)
fd7b16e391ed320e35255157a28be14c947ef30a test: Test dumps of other endian BDB files (Ava Chow)
6ace3e953f0864bd7818f040c59a1bc70aa47512 bdb: Be able to make byteswapped databases (Ava Chow)
d9878903fb34939dee8e1462f079acc68110253d Error if LSNs are not reset (Ava Chow)
4d7a3ae78e55f25868979f1bd920857a4aecb825 Berkeley RO Database fuzz test (TheCharlatan)
3568dce9e93295674cdf5458c5bdf93ff01fd0a2 tests: Add BerkeleyRO to db prefix tests (Ava Chow)
70cfbfdadf16d3b115309c6938f07ef5b96c7cc1 wallettool: Optionally use BERKELEY_RO as format when dumping BDB wallets (Ava Chow)
dd57713f6ede3d46e97ee7df87c10001b0bf4c3d Add MakeBerkeleyRODatabase (Ava Chow)
6e50bee67d1d58aecd8a0ce8b7c3f5a7979365f5 Implement handling of other endianness in BerkeleyRODatabase (Ava Chow)
cdd61c9cc108df8e13f4e3891ff2c96355b3ee38 wallet: implement independent BDB deserializer in BerkeleyRODatabase (Ava Chow)
ecba23097955dad7208baa687fc405c846aee794 wallet: implement BerkeleyRODatabase::Backup (Ava Chow)
0c8e72847603540bb29b8b8aeb80fa3f2e3a2c9a wallet: implement BerkeleyROBatch (Ava Chow)
756ff9b478484b17c4a6e65c171c2e4fecb21ad4 wallet: add dummy BerkeleyRODatabase and BerkeleyROBatch classes (Ava Chow)
ca18aea5c4975ace4e307be96c74641d203fa389 Add AutoFile::seek and tell (Ava Chow)
Pull request description:
Split from #26596
This PR adds `BerkeleyRODatabase` which is an independent implementation of a BDB file parser. It provides read only access to a BDB file, and can therefore be used as a read only database backend for wallets. This will be used for dumping legacy wallet records and migrating legacy wallets without the need for BDB itself.
Wallettool's `dump` command is changed to use `BerkeleyRODatabase` instead of `BerkeleyDatabase` (and `CWallet` itself) to demonstrate that this parser works and to test it against the existing wallettool functional tests.
ACKs for top commit:
josibake:
reACK d51fbab4b3
TheCharlatan:
Re-ACK d51fbab4b32d56765e8faab6ad01245fb259b0ca
furszy:
reACK d51fbab4b32d56765e8faab6ad01245fb259b0ca
laanwj:
re-ACK d51fbab4b32d56765e8faab6ad01245fb259b0ca
theStack:
ACK d51fbab4b32d56765e8faab6ad01245fb259b0ca
Tree-SHA512: 1e7b97edf223b2974eed2e9eac1179fc82bb6359e0a66b7d2a0c8b9fa515eae9ea036f1edf7c76cdab2e75ad994962b134b41056ccfbc33b8d54f0859e86657b
b47bd959207e82555f07e028cc2246943d32d4c3 kernel: De-globalize fReindex (TheCharlatan)
Pull request description:
fReindex is one of the last remaining globals exposed by the kernel library, so move it into the blockstorage class to reduce the amount of global mutable state and make the kernel library a bit less awkward to use.
---
This pull request is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587).
ACKs for top commit:
achow101:
ACK b47bd959207e82555f07e028cc2246943d32d4c3
ryanofsky:
Code review ACK b47bd959207e82555f07e028cc2246943d32d4c3. I rereviewed the whole PR, but the only change since last review was reverting the bugfix https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1578327024 and make the change a pure refactoring.
mzumsande:
Code Review ACK b47bd959207e82555f07e028cc2246943d32d4c3
stickies-v:
ACK b47bd959207e82555f07e028cc2246943d32d4c3
Tree-SHA512: f7399d01f93bc0c0c7428fe95d19b9d29b4ed00a4f1deabca78fb0c4fecb434ec971e890feecb105938b5247c926850b1b7b4a4a9caa333a061e40777d0c8463
9408a04e424cee0d226bde79171bd4954f9caeb0 tests, fuzz: use new NUMS_H const (josibake)
b946f8a4c51be42e52d63a6d578158c0b2a6b7ed crypto: add NUMS_H const (josibake)
Pull request description:
Broken out from #28122
---
[BIP341](https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki#constructing-and-spending-taproot-outputs) defines a NUMS point `H` as *H = lift_x(0x50929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac0)* which is [constructed](11af7015de/src/modules/rangeproof/main_impl.h (L16)) by taking the hash of the standard uncompressed encoding of the [secp256k1](https://www.secg.org/sec2-v2.pdf) base point G as X coordinate."
Add this as a constant so it can be used in our codebase. My primary motivation is BIP352 specifies a special case for when taproot spends use `H` as the internal key, but outside of BIP352 it seems generally useful to have `H` in the codebase, for testing or other use cases.
ACKs for top commit:
paplorinc:
re-ACK 9408a04e424cee0d226bde79171bd4954f9caeb0
achow101:
ACK 9408a04e424cee0d226bde79171bd4954f9caeb0
theStack:
Code-review ACK 9408a04e424cee0d226bde79171bd4954f9caeb0
Tree-SHA512: ad84492f5d635c0cb05bd82546079ded7e5138e95361f20d8285a9ad6e69c10ee2cc3fe46e16b46ef03c4253c8bee1051911c6b91264c90c3b1ad33a824bff4b
BerkeleyRODatabase is intended for use after BDB is removed, so it needs
to be able to read all of the records from a BDB file. Thus an
independent deserializer for BDB data files is implemented in it. This
deserializer is targeted towards the data files that Bitcoin Core
creates so it does not fully support all of BDB's features (e.g. other
database types, encryption, etc.).
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.
e41667b720372dae8438ea86e9819027e62b54e0 blockstorage: Don't move cursor backwards in UpdateBlockInfo (Ryan Ofsky)
17103637c6fa2dfcf5374ebb0cd715e540dd4ce1 blockstorage: Rename FindBlockPos and have it return a FlatFilePos (Martin Zumsande)
d9e477c4dc39d9623ed66c35c06e28f94ae62ad5 validation, blockstorage: Separate code paths for reindex and saving new blocks (Martin Zumsande)
064859bbad6984a6ec85c744064abdf757807c58 blockstorage: split up FindBlockPos function (Martin Zumsande)
fdae638e83522c28a1222e65c43d1cbca3e34cba doc: Improve doc for functions involved in saving blocks to disk (Martin Zumsande)
0d114e3cb20cb9e03fc9ba8daf3d03436b491742 blockstorage: Add Assume for fKnown / snapshot chainstate (Martin Zumsande)
Pull request description:
`SaveBlockToDisk` / `FindBlockPos` are used for two purposes, depending on whether they are called during reindexing (`dbp` set, `fKnown = true`) or in the "normal" case when adding new blocks (`dbp == nullptr`, `fKnown = false`).
The actual tasks are quite different
- In normal mode, preparations for saving a new block are made, which is then saved: find the correct position on disk (maybe skipping to a new blk file), check for available disk space, update the blockfile info db, save the block.
- during reindex, most of this is not necessary (the block is already on disk after all), only the blockfile info needs to rebuilt because reindex wiped the leveldb it's saved in.
Using one function with many conditional statements for this leads to code that is hard to read / understand and bug-prone:
- many code paths in `FindBlockPos` are conditional on `fKnown` or `!fKnown`
- It's not really clear what actually needs to be done during reindex (we don't need to "save a block to disk" or "find a block pos" as the function names suggest)
- logic that should be applied to only one of the two modes is sometimes applied to both (see first commit, or #27039)
#24858 and #27039 were recent bugs directly related to the differences between reindexing and normal mode, and in both cases the simple fix took a long time to be reviewed and merged.
This PR proposes to clean this code up by splitting out the reindex logic into a separate function (`UpdateBlockInfo`) which will be called directly from validation. As a result, `SaveBlockToDisk` and `FindBlockPos` only need to cover the non-reindex logic.
ACKs for top commit:
paplorinc:
ACK e41667b720372dae8438ea86e9819027e62b54e0
TheCharlatan:
Re-ACK e41667b720372dae8438ea86e9819027e62b54e0
ryanofsky:
Code review ACK e41667b720372dae8438ea86e9819027e62b54e0. Just improvements to comments since last review.
Tree-SHA512: a14ff9a0facf6b1e3c1cd724a2d19a79a25d4b48de64398fdd172671532a472bc10a20cbb64ac3a3e55814dcc877d0597a3e1699cabc4f9d9a86b439b6eaba20
cbc6c440e3811d342fa570713702900b3e3e75b9 doc: add comments and release-notes for JSON-RPC 2.0 (Matthew Zipkin)
e7ee80dcf2b68684eae96070875ea13a60e3e7b0 rpc: JSON-RPC 2.0 should not respond to "notifications" (Matthew Zipkin)
bf1a1f1662427fbf1a43bb951364eface469bdb7 rpc: Avoid returning HTTP errors for JSON-RPC 2.0 requests (Matthew Zipkin)
466b90562f4785de74b548f7c4a256069e2aaf43 rpc: Add "jsonrpc" field and drop null "result"/"error" fields (Matthew Zipkin)
2ca1460ae3a7217eaa8c5972515bf622bedadfce rpc: identify JSON-RPC 2.0 requests (Matthew Zipkin)
a64a2b77e09bff784a2635ba19ff4aa6582bb5a5 rpc: refactor single/batch requests (Matthew Zipkin)
df6e3756d6feaf1856e7886820b70874209fd90b rpc: Avoid copies in JSONRPCReplyObj() (Matthew Zipkin)
09416f9ec445e4d6bb277400758083b0b4e8b174 test: cover JSONRPC 2.0 requests, batches, and notifications (Matthew Zipkin)
4202c170da37a3203e05a9f39f303d7df19b6d81 test: refactor interface_rpc.py (Matthew Zipkin)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/2960
Bitcoin Core's JSONRPC server behaves with a special blend of 1.0, 1.1 and 2.0 behaviors. This introduces compliance issues with more strict clients. There are the major misbehaviors that I found:
- returning non-200 HTTP codes for RPC errors like "Method not found" (this is not a server error or an HTTP error)
- returning both `"error"` and `"result"` fields together in a response object.
- different error-handling behavior for single and batched RPC requests (batches contain errors in the response but single requests will actually throw HTTP errors)
https://github.com/bitcoin/bitcoin/pull/15495 added regression tests after a discussion in https://github.com/bitcoin/bitcoin/pull/15381 to kinda lock in our RPC behavior to preserve backwards compatibility.
https://github.com/bitcoin/bitcoin/pull/12435 was an attempt to allow strict 2.0 compliance behind a flag, but was abandoned.
The approach in this PR is not strict and preserves backwards compatibility in a familiar bitcoin-y way: all old behavior is preserved, but new rules are applied to clients that opt in. One of the rules in the [JSON RPC 2.0 spec](https://www.jsonrpc.org/specification#request_object) is that the kv pair `"jsonrpc": "2.0"` must be present in the request. Well, let's just use that to trigger strict 2.0 behavior! When that kv pair is included in a request object, the [response will adhere to strict JSON-RPC 2.0 rules](https://www.jsonrpc.org/specification#response_object), essentially:
- always return HTTP 200 "OK" unless there really is a server error or malformed request
- either return `"error"` OR `"result"` but never both
- same behavior for single and batch requests
If this is merged next steps can be:
- Refactor bitcoin-cli to always use strict 2.0
- Refactor the python test framework to always use strict 2.0 for everything
- Begin deprecation process for 1.0/1.1 behavior (?)
If we can one day remove the old 1.0/1.1 behavior we can clean up the rpc code quite a bit.
ACKs for top commit:
cbergqvist:
re ACK cbc6c440e3811d342fa570713702900b3e3e75b9
ryanofsky:
Code review ACK cbc6c440e3811d342fa570713702900b3e3e75b9. Just suggested changes since the last review: changing uncaught exception error code from PARSE_ERROR to MISC_ERROR, renaming a few things, and adding comments.
tdb3:
re ACK for cbc6c440e3811d342fa570713702900b3e3e75b9
Tree-SHA512: 0b702ed32368b34b29ad570d090951a7aeb56e3b0f2baf745bd32fdc58ef68fee6b0b8fad901f1ca42573ed714b150303829cddad4a34ca7ad847350feeedb36
fReindex is one of the last remaining globals exposed by the kernel
library, so move it into the blockstorage class to reduce the amount of
global mutable state and make the kernel library a bit less awkward to
use.
d0b047494c28381942c09d0cca45baa323bfcffc test: add GetAddedNodeInfo() CJDNS regression unit test (Jon Atack)
684da9707040ce25d95b2954eda50b811136d92c p2p, bugfix: detect addnode cjdns peers in GetAddedNodeInfo() (Jon Atack)
Pull request description:
Addnode peers connected to us via the cjdns network are currently not detected by `CConnman::GetAddedNodeInfo()`, i.e. `fConnected` is always false. This causes the following issues:
- RPC `getaddednodeinfo` incorrectly shows them as not connected
- `CConnman::ThreadOpenAddedConnections()` continually retries to connect them
Fix the issue and add a unit regression test. Extracted from #28248. Suggest running the test with:
`./src/test/test_bitcoin -t net_peer_connection_tests -l test_suite`
ACKs for top commit:
mzumsande:
utACK d0b047494c28381942c09d0cca45baa323bfcffc
brunoerg:
crACK d0b047494c28381942c09d0cca45baa323bfcffc
pinheadmz:
ACK d0b047494c28381942c09d0cca45baa323bfcffc
Tree-SHA512: a4d81425f79558f5792585611f3fe8ab999b82144daeed5c3ec619861c69add934c2b2afdad24c8488a0ade94f5ce8112f5555d60a1ce913d4f5a1cf5dbba55a
06bff6dec8 Merge bitcoin-core/secp256k1#1528: tests: call `secp256k1_ecmult_multi_var` with a non-`NULL` error callback
4155e62fcc Merge bitcoin-core/secp256k1#1526: cmake: Fix `check_arm32_assembly` when using as subproject
9554362b15 tests: call secp256k1_ecmult_multi_var with a non-NULL error callback
9f4c8cd730 cmake: Fix `check_arm32_assembly` when using as subproject
7712a53061 Merge bitcoin-core/secp256k1#1524: check-abi: explicitly provide public headers
7d0bc0870f Merge bitcoin-core/secp256k1#1525: changelog: Correct 0.5.0 release date
d45d9b74bb changelog: Correct 0.5.0 release date
d7f6613dbb Merge bitcoin-core/secp256k1#1523: release cleanup: bump version after 0.5.0
2f05e2da4b release cleanup: bump version after 0.5.0
e3a885d42a Merge bitcoin-core/secp256k1#1522: release: prepare for 0.5.0
dd695563e6 check-abi: explicitly provide public headers
c0e4ec3fee release: prepare for 0.5.0
bb528cfb08 Merge bitcoin-core/secp256k1#1518: Add secp256k1_pubkey_sort
7d2591ce12 Add secp256k1_pubkey_sort
da515074e3 Merge bitcoin-core/secp256k1#1058: Signed-digit multi-comb ecmult_gen algorithm
4c341f89ab Add changelog entry for SDMC
a043940253 Permit COMB_BITS < 256 for exhaustive tests
39b2f2a321 Add test case for ecmult_gen recoded = {-1,0,1}
644e86de9a Reintroduce projective blinding
07810d9abb Reduce side channels from single-bit reads
a0d32b597d Optimization: use Nx32 representation for recoded bits
e03dcc44b5 Make secp256k1_scalar_get_bits support 32-bit reads
5005abee60 Rename scalar_get_bits -> scalar_get_bits_limb32; return uint32_t
6247f485b6 Optimization: avoid unnecessary doublings in precomputation
15d0cca2a6 Optimization: first table lookup needs no point addition
7a33db35cd Optimization: move (2^COMB_BITS-1)/2 term into ctx->scalar_offset
ed2a056f3d Provide 3 configurations accessible through ./configure
5f7be9f6a5 Always generate tables for current (blocks,teeth) config
fde1dfcd8d Signed-digit multi-comb ecmult_gen algorithm
486518b350 Make exhaustive tests's scalar_inverse(&x,&x) work
ab45c3e089 Initial gej blinding -> final ge blinding
aa00a6b892 Introduce CEIL_DIV macro and use it
git-subtree-dir: src/secp256k1
git-subtree-split: 06bff6dec8d038f7b4112664a9b882293ebc5178
141df0a28810470e53fdbc6d32d3cb4020fe3ca1 crypto: disable asan for sha256_sse4 with clang and -O0 (Cory Fields)
Pull request description:
Clang is unable to compile the Transform function for that combination of options.
Fixes#29801.
ACKs for top commit:
achow101:
ACK 141df0a28810470e53fdbc6d32d3cb4020fe3ca1
Tree-SHA512: d74fdac5840ad7524edfde069fb43ae75c31146e90ecc58bbc7912ff57a02b068547431b1766afeed782272c0b93b0b41a286c1cf26ec55ce332d94ce917d810
