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
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
Replace ExpandHRP with a PreparePolynomialCoefficients function. Instead
of using a hardcoded value for the size of the array (90 in this case)
and a hardcoded value for the checksum, use the actual values vector and
define checksum size as a constexpr. Use the new CHECKSUM_SIZE
throughout instead 6.
Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
8950053636cb38ed85fe2d58b53e5d0acb35c390 test: remove unneeded `-maxorphantx=1000` settings (Sebastian Falbesoner)
Pull request description:
It's unclear what the motivation for increasing the orphan pool is here, and it seems that this not needed at all. None of these tests involve orphan transactions explicitly, and if they would occur occasionally, there is no good reason to prefer a value of 1000 over the default of 100 (see DEFAULT_MAX_ORPHAN_TRANSACTIONS).
ACKs for top commit:
maflcko:
utACK 8950053636cb38ed85fe2d58b53e5d0acb35c390
edilmedeiros:
Tested ACK 8950053636cb38ed85fe2d58b53e5d0acb35c390
AngusP:
tACK 8950053636cb38ed85fe2d58b53e5d0acb35c390
glozow:
ACK 8950053636cb38ed85fe2d58b53e5d0acb35c390 From skimming the tests, it appears that none of these need a larger `-maxorphantx`.
Tree-SHA512: 81d4a4fb2ea92b97119f21cbc6c4b1240d863269932e6adf4982aead9726f20652523a4707add3ad38eb332d4452de41de6735265f22e62298f3b4b45de75a57
9365baa489e123d9bcaf986e4311d3fa3f1e3f88 test: add conflicting topology test case (Greg Sanders)
Pull request description:
We want to ensure that even if topologies
that are acceptable are relaxed, like
removing package-not-child-with-unconfirmed-parents, that we don't end up accepting packages we shouldn't.
ACKs for top commit:
glozow:
reACK 9365baa489
rkrux:
reACK [9365baa](9365baa489)
Tree-SHA512: d58661064ca099ac0447c331a5020c74c0cdfe24259aa875592805bbd63de1bf23aa7ced9ff485fef90dc0602fcb997e631aaf1aa2e9805d2cf5f0e5c9b2f0e2
It's unclear what the motivation for increasing the orphan pool is, and
it seems that this not needed at all. None of these tests involve orphan
transactions explicitly, and if they would occur occasionally, there is
no good reason to prefer a value of 1000 over the default of 100 (see
DEFAULT_MAX_ORPHAN_TRANSACTIONS).
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
Since the wallet is appended to the global WalletContext during
creation, merely calling 'reset()' on the benchmark shared_pointer
is insufficient to destruct the wallet. This no destruction of the
wallet results in the db connection remaining open, which was the
cause of the 'fs::remove_all()' error in Windows.
Co-Authored-By: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
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
There are no changes to behavior. Changes in this commit are all additions, and
are easiest to review using "git diff -U0 --word-diff-regex=." options.
Motivation for this change is to keep util functions with really generic names
like "Split" and "Join" out of the global namespace so it is easier to see
where these functions are defined, and so they don't interfere with function
overloading, especially since the util library is a dependency of the kernel
library and intended to be used with external code.
Add TransactionError to node namespace and include it directly instead of
relying on indirect include through common/messages.h
This is a followup to a previous commit which moved the TransactionError enum.
These changes were done in a separate followup just to keep the previous commit
more minimal and easy to review.
Move enum and message formatting functions to a common/messages header where
they should be more discoverable, and also out of the util library, so they
will not be a dependency of the kernel
The are no changes in behavior and no changes to the moved code.
Add separate PSBTError enum instead of reusing TransactionError enum for PSBT
operations, and drop unused error codes. The error codes returned by PSBT
operations and transaction broadcast functions mostly do not overlap, so using
an unified enum makes it harder to call any of these functions and know which
errors actually need to be handled.
Define PSBTError in the common library because PSBT functionality is
implemented in the common library and used by both the node (for rawtransaction
RPCs) and the wallet.
New node/types.h file is analagous to existing wallet/types.h and is a better
place to define simple node types that are shared externally with wallet and
gui code than the util library.
Motivation for this change is to completely remove util/error.h file currently
holding TransactionError in a followup commit.
Move miniscript / descriptor script parsing functions out of util library so
they are not a dependency of the kernel.
There are no changes to code or behavior.
This will help move the miniscript / descriptor parsing functions out of the
util library in an upcoming commit, so they are not exposed to libbitcoinkernel
applications. Moving the Split functions should also make them more
discoverable since they now close to related functions like Join.
The functions are moved verbatim without any changes.
Move HexStr and HexDigit functions from util to crypto. The crypto library does
not actually use these functions, but the consensus library does. The consensus
and util libraries not allowed to depend on each other, but are allowed to
depend on the cryto library, so the crypto library is a reasonable put these.
The consensus library uses HexStr and HexDigit in script.cpp, transaction.cpp,
and uint256.cpp.
The util library does not use HexStr but does use HexDigit in strencodings.cpp
to parse integers.
Move util/message to common/signmessage so it is named more clearly, and
because the util library is not supposed to depend on other libraries besides
the crypto library. The signmessage functions use CKey, CPubKey, PKHash, and
DecodeDestination functions in the consensus and common libraries.
