Age | Commit message (Collapse) | Author |
|
bb1c84082c8a8709fec201f5768deab22c938241 Remove the build_msvc/testconsensus project (Aaron Clauson)
Pull request description:
The testconsensus project is not integral to the Bitcoin Core code. It was originally added as a quick and dirty demo of how to do a consensus check with the msvc build. There are better examples.
PR #23438 made a change that caused a compiler error with the buildmsvc/testconsensus code. Rather than leave it hanging around to incur potential bitrot, or further PR build failures, it should be removed.
ACKs for top commit:
hebasto:
ACK bb1c84082c8a8709fec201f5768deab22c938241, tested on Windows 10 Pro (20H2).
Tree-SHA512: d81b99eb09171b66c179961b15f0b2e2e97e5ee7f011f18667e890c90e3d169593ad9aedd05a8616e962212952048827b7159d3c2a2ecb7ac378136b80bf6b23
|
|
|
|
The testconsensus project is not integral to the Bitcoin Core code. It was originally added as a quick and dirty demo of how to do a consensus check with the msvc build. There are better examples.
PR #23438 made a change that caused a compiler error with the buildmsvc/testconsensus code. Rather than leave it hanging around to incur potential bitrot, or furhter PR build failures, it should be removed.
|
|
See https://github.com/bitcoin-core/bitcoincore.org/pull/809 and
https://github.com/bitcoin-core/bitcoincore.org/pull/819 and
https://bitcoincore.org/en/contact/.
|
|
This updates build-unix for the descriptor wallet, and prepares for
eventual legacy wallet deprecation.
- Move 'descriptor wallet' dependencies above legacy wallet deps both
for Debian and Fedora.
- Explicitly mention 'legacy wallet' where referring to the BerkeleyDB
wallet. Shorten BerkeleyDB instruction to a single paragraph.
|
|
11115169a14d0d0be5b7b1c3f6fdc9673a9098d9 ci: Build fuzz with libsqlite3-dev (MarcoFalke)
fa7c6efca66627e4c76adecc824f96da220af69c fuzz: Add wallet fuzz test (MarcoFalke)
fa59d2ce5b8d6fe8c610f170a13675c756aef58f refactor: Use local args instead of global gArgs in CWallet::Create (MarcoFalke)
fadb44606f26a80daf4320eee046c9572e85fe3e build: Inline FUZZ_SUITE_LDFLAGS_COMMON (MarcoFalke)
Pull request description:
Initial sketch to fuzz descriptor wallets. Can be improved in the future.
ACKs for top commit:
mjdietzx:
Code review ACK 1111516
Tree-SHA512: b1d2f24504d1ed5f3c6a031210f04c27c13d4e15576c4acbf50ded37ac45f7b7a5c7553e91d81d4a06e9ea73b3d745a552218d3ef3b2932fa5325a8331b0d3fd
|
|
Makes the test more minimal. We're just trying to test that our package
sanitization logic is correct. Now that this code lives in its own
function (rather than inside of AcceptMultipleTransactions), there's no
need to call ProcessNewPackage to test this.
|
|
|
|
No change in behavior, because package transactions would not be going
through the rbf logic in PreChecks anyway (BIP125 is currently disabled
for package acceptance, see ATMPArgs).
We draw the line here because each individual transaction in package
validation still goes through all PreChecks. For example, checking that
one's own conflicts and dependencies are disjoint (a consensus check)
and individual transaction mempool ancestor/descendant limits.
|
|
The aliases are leftover from a previous MOVEONLY refactor - they are
unnecessary and removing them reduces the diff for splitting out mempool
Checks from PreChecks, making RBF variables MemPoolAccept-wide, etc.
-BEGIN VERIFY SCRIPT-
unalias() { sed -i "s:\<$1\>:$2:g" src/validation.cpp; sed -i "/$2 = $2/d" src/validation.cpp; }
unalias nModifiedFees ws.m_modified_fees
unalias nConflictingFees ws.m_conflicting_fees
unalias nConflictingSize ws.m_conflicting_size
unalias setConflicts ws.m_conflicts
unalias allConflicting ws.m_all_conflicting
unalias setAncestors ws.m_ancestors
-END VERIFY SCRIPT-
|
|
The tests `streams_tests/streams_buffered_file` and `streams_tests/streams_buffered_file_rand`
did not use a the temporary directory provided by `BasicTestingSetup`, so it was not possible
to execute multiple of them in parallel. This fixes that.
To reproduce, run
```sh
parallel --halt now,fail=1 './src/test/test_bitcoin --run_test=streams_tests/streams_buffered_file_rand' -- ::: {1..1000}
```
This executes the test 1000 times, one job per CPU. It works on that commit but mergebase fails quickly.
|
|
|
|
|
|
than expected feerate
80dc829be7f8c3914074b85bb4c125baba18cb2c tests: Calculate fees more similarly to CFeeRate::GetFee (Andrew Chow)
ce2cc44afd51f3df4ee7f14ea05b8da229183923 tests: Test for assertion when feerate is rounded down (Andrew Chow)
0fbaef9676a1dcb84bcf95afd8d994831ab327b6 fees: Always round up fee calculated from a feerate (Andrew Chow)
Pull request description:
When calculating the fee for a feerate, it is possible that the final calculation will have fractional satoshis. Currently those are ignored via truncation which results in the absolute fee being rounded down. Rounding down is problematic because it results in a feerate that is slightly lower than the feerate represented by the `CFeeRate` object. A slightly lower feerate particularly causes issues for coin selection as it can trigger an assertion error. To avoid potentially underpaying the feerate (and the assertion), always round up the calculated fee.
A test is added for the assertion, along with a comment explaining what happens.
It is unlikely that a user can trigger this as it requires a very specific set of rounding errors to occur as well as the transaction not needing any change and being right on the lower bound of the exact match window. However I was able to trigger the assertion while running coin selection simulations, albeit after thousands of transactions and with some weird feerates.
ACKs for top commit:
ryanofsky:
Code review ACK 80dc829be7f8c3914074b85bb4c125baba18cb2c
promag:
Tested ACK 80dc829be7f8c3914074b85bb4c125baba18cb2c.
lsilva01:
tACK 80dc829
meshcollider:
utACK 80dc829be7f8c3914074b85bb4c125baba18cb2c
Tree-SHA512: fe26684c60f236cab48ea6a4600c141ce766dbe59504ec77595dcbd7fd0b34559acc617007f4f499c9155d8fda0a336954413410ba862b19c765c0cfac79d642
|
|
CTxMemPool::check() will carry out internal consistency checks 1/n times,
where n is set by the `-checkmempool` configuration option. By default,
mempool consistency checks are disabled entirely on mainnet.
Therefore, this change has no effect on mainnet nodes running with
default configuration. It simply removes the responsibility to trigger
mempool consistency checks from net_processing.
|
|
|
|
This just calls through to AcceptToMemoryPool() internally, and is currently unused.
Also add a new transaction validation failure reason TX_NO_MEMPOOL to
indicate that there is no mempool.
|
|
User-facing error messages should not leak internal implementation
details like function names. Update the MEMPOOL_REJECTED error string
from "Transaction rejected by AcceptToMemoryPool" to the more generic
"Transaction rejected by mempool". Also update the MEMPOOL_ERROR error
message from "AcceptToMemoryPool failed" to the more precise "Mempool
internal error" since this error indicates and internal (e.g.
logic/hardware/etc) failure, and not a transaction rejection.
|
|
"This logic is not necessary for memory pool transactions, as
AcceptToMemoryPool already refuses previously-known transaction ids
entirely." refers to the logic at
https://github.com/bitcoin/bitcoin/blob/a206b0ea12eb4606b93323268fc81a4f1f952531/src/main.cpp#L484-L486,
which was later removed in commit 450cbb0944cd20a06ce806e6679a1f4c83c50db2.
|
|
An IPv6 address from fc00::/8 could be either from the CJDNS network or
from a private-unroutable-reserved segment of IPv6. A seed node with
such an address must be from the CJDNS network, otherwise other peers
will not be able to connect to it.
|
|
CJDNS addresses start with constant 8 bits, so in order to account for
the first 4 random ones, we must take the first 12. Otherwise the entire
CJDNS network will belong to one group.
|
|
This will help with propagation, so that multi-homed nodes can learn
CJDNS addresses outside of the CJDNS network.
|
|
|
|
This way `GetLocal()` will pick our CJDNS address for a CJDNS peer.
|
|
|
|
The default bool argument makes it harder to read because the last but
one argument is also bool. Pass all of them as named arguments to
increase readability.
Another bool argument will be added to indicate whether to test CJDNS.
Co-authored-by: Jon Atack <jon@atack.com>
|
|
In some cases addresses come from an external source as a string or as a
`struct sockaddr_in6`, without a tag to tell whether it is a private
IPv6 or a CJDNS address. In those cases interpret the address as a CJDNS
address instead of an IPv6 address if `-cjdnsreachable` is set and the
seemingly-IPv6-address belongs to `fc00::/8`. Those external sources are:
* `-externalip=`
* `-bind=`
* UPnP
* `getifaddrs(3)` (called through `-discover`)
* `addnode`
* `connect`
* incoming connections (returned by `accept(2)`)
|
|
|
|
CJDNS is set up in the host OS, outside of the application. When the
routing is configured properly then connecting to fc00::/8 results in
connecting to the CJDNS network.
Introduce an option so that Bitcoin Core knows whether this is the case.
|
|
This allows to use "cjdns" as an argument to the `getnodeaddresses` RPC
and to the `-onlynet=` parameter.
|
|
If `-proxy` is given, then also use it for connecting to the CJDNS
network.
|
|
Connecting to CJDNS addresses works without a proxy, just like
connecting to an IPv6 address. Thus adapt `CService::GetSockAddr()` to
retrieve the `struct sockaddr*` even for `CService::IsCJDNS()` objects.
|
|
AcceptToMemoryPool() is called for an invalid coinbase transaction, so
setting bypass_limits to true or false has no impact on the test.
The only way that changing bypass_limits from true to false could change
the result would be to change the outcome to INVALID(TX_MEMPOOL_POLICY).
Since the ATMP call in this test results in INVALID(TX_CONSENSUS) both
before and after this change, there is no change in behavior.
|
|
AcceptToMemoryPool() is called for transactions with fees above
minRelayTxFee and with the mempool not full, so setting bypass_limits to
true or false has no impact on the test.
The only way that changing bypass_limits from true to false could change
the result would be to change the outcome to INVALID(TX_MEMPOOL_POLICY).
Since all the ATMP calls in this test result in VALID both before and
after this change, there is no change in behavior.
|
|
9ab440199d5c888363a42c957433d0e46cd0d2ff doc: add assumeutxo notes (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: #15606)
---
Adds some notes on assumeutxo design.
Related: https://github.com/bitcoin/bitcoin/pull/21526#discussion_r715558994
ACKs for top commit:
ariard:
ACK 9ab4401
naumenkogs:
ACK 9ab4401
michaelfolkson:
ACK 9ab440199d5c888363a42c957433d0e46cd0d2ff
fjahr:
ACK 9ab440199d5c888363a42c957433d0e46cd0d2ff
Tree-SHA512: 2fca8373b78701754957d12bc43ce18aa6928507965448741cb4e8c56589ad61d261f8542e348094fc9631d46ee6a7afee75c965c0db993fc816758569137b74
|
|
txmempool.h to .cpp file
65aaf9495d19ea3fb875228a7e14aab6c1f2986d refactor: move `update_*` structs from txmempool.h to .cpp file (Sebastian Falbesoner)
9947ce62626c05bd186ae8a4864aa382f673ec1a refactor: use const reference for parents in `CTxMemPool::UpdateAncestorsOf` (Sebastian Falbesoner)
Pull request description:
These helpers are exclusively used in txmempool.cpp, hence they should also be moved there. The PR also contains a commit which fixes const-correctness for parents in `CTxMemPool::UpdateAncestorsOf` and declares them as reference to avoid a copy.
ACKs for top commit:
promag:
Code review ACK 65aaf9495d19ea3fb875228a7e14aab6c1f2986d. Verified move-only commit locally.
Tree-SHA512: 7ce29f3ba0e68b5355001f27725b00f6d54cc993015356eb40b61b8cdd17db49b980f4c3d798c8e0c940d245dc3a72c474bb9ff3c0ee971ead450786076812c2
|
|
6ae9f1cf9604227e9dfda1f6c91fc711d154362e Disable lock contention logging in checkqueue_tests (Jon Atack)
Pull request description:
This patch disables lock contention logging in the checkqueue_tests as some of these tests are designed to be heavily contested to trigger race conditions or other issues. This created very large log files when run with DEBUG_LOCKCONTENTION defined (up to v22) or with lock logging enabled by default in current master.
Examples running the following command:
```
$ ./src/test/test_bitcoin -t checkqueue_tests/test_CheckQueue_Correct_Random -- DEBUG_LOG_OUT > testlog.txt
-rw-r--r-- 87042178 Oct 8 12:41 testlog-with-DEBUG_LOCKCONTENTION-at-v22-run1.txt
-rw-r--r-- 73879896 Oct 8 12:42 testlog-with-DEBUG_LOCKCONTENTION-at-v22-run2.txt
-rw-r--r-- 65150518 Oct 8 12:51 testlog-with-DEBUG_LOCKCONTENTION-at-bb9f76a-run1.txt
-rw-r--r-- 65774554 Oct 8 12:52 testlog-with-DEBUG_LOCKCONTENTION-at-bb9f76a-run2.txt
-rw-r--r-- 73493309 Oct 8 13:00 testlog-current-master-at-991753e-run1.txt
-rw-r--r-- 65616977 Oct 8 13:01 testlog-current-master-at-991753e-run2.txt
-rw-r--r-- 5093 Oct 8 13:04 testlog-with-this-commit-run1.txt
-rw-r--r-- 5093 Oct 8 13:05 testlog-with-this-commit-run2.txt
```
Resolves #23167.
ACKs for top commit:
vasild:
ACK 6ae9f1cf9604227e9dfda1f6c91fc711d154362e
Tree-SHA512: b16812ed60c58a1cf40c04ebeca9197ac076b2415f71673ac7bb5b7960a1ff80ba2c909345ad221c7689b0562d17f63a32a629f5d6dbcf0e57130bf5760388c1
|
|
As defined in BIP340, a verification step should be executed after
`secp256k1_schnorrsig_sign` to ensure that a potentially corrupted
signature isn't used; using corrupted signatures could reveal
information about the private key used. This applies to ECSDA as
well.
Additionally clears schnorr signature if signing failed.
|
|
TransportDeserializer::GetMessage()
f3e451bebfe2e2d8de901d8ac29c064a51d3b746 [net] Replace GetID() with id in TransportDeserializer constructor (Troy Giorshev)
8c96008ab18075abca03bff6b3675643825a21ca [net] Don't return an optional from TransportDeserializer::GetMessage() (Troy Giorshev)
Pull request description:
Also, access mapRecvBytesPerMsgCmd with `at()` not `find()`. This
throws an error if COMMAND_OTHER doesn't exist, which should never
happen. `find()` instead just accessed the last element, which could make
debugging more difficult.
Resolves review comments from PR19107:
- https://github.com/bitcoin/bitcoin/pull/19107#discussion_r478718436
- https://github.com/bitcoin/bitcoin/pull/19107#discussion_r478714497
ACKs for top commit:
theStack:
Code-review ACK f3e451bebfe2e2d8de901d8ac29c064a51d3b746
ryanofsky:
Code review ACK f3e451bebfe2e2d8de901d8ac29c064a51d3b746. Changes since last review in https://github.com/bitcoin/bitcoin/pull/20364#pullrequestreview-534369904 were simplifying by dropping the third commit, rebasing, and cleaning up some style & comments in the first commit.
Tree-SHA512: 37de4b25646116e45eba50206e82ed215b0d9942d4847a172c104da4ed76ea4cee29a6fb119f3c34106a9b384263c576cb8671d452965a468f358d4a3fa3c003
|
|
c02a674e97b61f675c94341fca1bf232070e6991 doc: Add output script descriptors BIPs 380..386 (Hennadii Stepanov)
Pull request description:
BIPs 380..385 are implemented as of v0.17.0.
BIP 386 is implemented as of v22.0.
ACKs for top commit:
sipa:
ACK c02a674e97b61f675c94341fca1bf232070e6991
jarolrod:
ACK c02a674e97b61f675c94341fca1bf232070e6991
shaavan:
ACK c02a674e97b61f675c94341fca1bf232070e6991
Tree-SHA512: 40f0252d3aad08c61a8e1476d26a590dbcf3f9d66c1f8315d15d13feb17288cc25b9c75df5b938f77695eafaba847dacc0020a880ba6034a511e7c9b7f40fd8f
|
|
|
|
psbt_wallet_tests/psbt_updater_test
68018e4c3e76f7e5bebf5f90ffd972c7bf01e0a0 test: Avoid excessive locking of `cs_wallet` (Hennadii Stepanov)
7986faf2e09ea85b1d4564ce910f07a4c4de8685 test: Fix segfault in the psbt_wallet_tests/psbt_updater_test (Hennadii Stepanov)
Pull request description:
The dcd6eeb64adb2b532f5003cbb86ba65b3c08a87b commit (bitcoin/bitcoin#23288) introduced an intermittent failure in the `psbt_wallet_tests/psbt_updater_test` unit test. See bitcoin/bitcoin#23368.
The test failure can be easily made reproducible with the following patch:
```diff
--- a/src/scheduler.cpp
+++ b/src/scheduler.cpp
@@ -57,6 +57,8 @@ void CScheduler::serviceQueue()
Function f = taskQueue.begin()->second;
taskQueue.erase(taskQueue.begin());
+ UninterruptibleSleep(100ms);
+
{
// Unlock before calling f, so it can reschedule itself or another task
// without deadlocking:
```
This PR implements an idea which was mentioned in the [comment](https://github.com/bitcoin/bitcoin/issues/23368#issuecomment-953796339):
> Yes, as I said before this looks like a race where the wallet is deleted before stopping the scheduler: [#23368 (comment)](https://github.com/bitcoin/bitcoin/issues/23368#issuecomment-952808824)
>
> IIRC, the order should be:
>
> * stop scheduler
>
> * delete wallet
>
> * delete scheduler
The second commit introduces a refactoring with no behavior change.
Fixes bitcoin/bitcoin#23368.
ACKs for top commit:
mjdietzx:
Code review ACK 68018e4c3e76f7e5bebf5f90ffd972c7bf01e0a0
Tree-SHA512: d9103f6252aab807453628159dec243bc543a2595eecaa04ec761dca3c2370085592c55d6f50967d69a4ac6e8b5827eec30dd9b025132c99b0bb9aa5911ad915
|
|
This makes calling code less verbose and less fragile. Also, by adding
the CKey::data() member function, it is now possible to call HexStr()
with a CKey object.
|
|
ArgsManager flags
c5d7e34bd9a4ad752c5ec88032420e2e90ab17ab scripted-diff: disable unimplemented ArgsManager BOOL/INT/STRING flags (Russell Yanofsky)
b8c069b7a952e326d2d974cc671889d1a3b38aa4 refactor: Add explicit DISALLOW_NEGATION ArgsManager flag to clarify flag usage (Russell Yanofsky)
26a50ab322614bceb5bc62e2c282f83e5987bad8 refactor: Split InterpretOption into Interpret{Key,Value} functions (Russell Yanofsky)
Pull request description:
This is preparation for #16545 or another PR implementing type validation for ArgsManager settings. It fixes misleading usages of existing flags, prevents flags from being similarly misused in the future, and allows validation logic to be added without breaking backwards compatibility.
---
Currently, ALLOW_{INT|BOOL|STRING} flags don't do any real validation, so current uses of these flags are misleading and will also break backwards compatibility whenever these flags are implemented in a future PR (draft PR is #16545).
An additional complication is that while these flags don't do any real settings validation, they do affect whether setting negation syntax is allowed.
Fix this mess by disabling ALLOW_{INT|BOOL|STRING} flags until they are implemented, and adding an unambiguous DISALLOW_NEGATION flag. This is done in three commits, with the first commit cleaning up some code, the second commit adding the DISALLOW_NEGATION flag, and the next commit disabling the ALLOW_{INT|BOOL|STRING} flags.
None of the changes affect behavior in any way.
ACKs for top commit:
ajtowns:
utACK c5d7e34bd9a4ad752c5ec88032420e2e90ab17ab
promag:
Code review ACK c5d7e34bd9a4ad752c5ec88032420e2e90ab17ab, which as the new argument `-legacy`.
Tree-SHA512: cad0e06361e8cc584eb07b0a1f8b469e3beea18abb458c4e43d9d16e9f301b12ebf1d1d426a407fbd96f99724ad6c0eae5be05c713881da7c55e0e08044674eb
|
|
and logging
61ec0539b26a902a41a2602187a71f9dba3c6935 [MOVEONLY] reorder functions in addrman_impl.h and addrman.cpp (John Newbery)
2095df7b7bfcb9ab0c5710a93112f7f341e753c9 [addrman] Add Add_() inner function, fix Add() return semantics (John Newbery)
2658eb6d68460272deefb3fcc653b03f6ec6e7cf [addrman] Rename Add_() to AddSingle() (John Newbery)
e58598e833d5737900fe3c4369e26f2a08166892 [addrman] Add doxygen comment to AddrMan::Add() (John Newbery)
Pull request description:
Previously, Add() would return true if the function created a new
AddressInfo object, even if that object could not be successfully
entered into the new table and was deleted. That would happen if the new
table position was already taken and the existing entry could not be
removed.
Instead, return true if the new AddressInfo object is successfully
entered into the new table. This fixes a bug in the "Added %i addresses"
log, which would not always accurately log how many addresses had been
added.
ACKs for top commit:
naumenkogs:
ACK 61ec0539b26a902a41a2602187a71f9dba3c6935
mzumsande:
ACK 61ec0539b26a902a41a2602187a71f9dba3c6935
shaavan:
ACK 61ec0539b26a902a41a2602187a71f9dba3c6935
Tree-SHA512: 276f1e8297d4b6d411d05d06ffc7c176f6290a784da039926ab6c471a8ed8e9159ab4f56c893b1285737ae292954930f0d28012d89dfb3f2f825d7df41016feb
|
|
|
|
The bug was introduced in dcd6eeb64adb2b532f5003cbb86ba65b3c08a87b.
|
|
9ba7c44265a47880585e39d0167d057ba935ff16 refactor: get wallet path relative to wallet_dir (Michael Dietz)
Pull request description:
Now that boost has been updated > 1.60 (see #22320), we can simplify how we get
wallet path relative to wallet_dir by using:
`boost::filesystem::lexically_relative`, removing a TODO.
Test coverage comes from `test/functional/wallet_multiwallet.py`
I first tried this in #20265 which was my first attempted PR, and funny enough exactly 1 year later I'm opening this one to hopefully finally close this.
ACKs for top commit:
ryanofsky:
Code review ACK 9ba7c44265a47880585e39d0167d057ba935ff16. Basically this same code change is made in #20744 commit b70c84348ac7a8e427a1183f894c73e52c734529, so this PR helps simplify that one
lsilva01:
Code Review ACK 9ba7c44
Tree-SHA512: 6ccb91a18bcb52c3ae0c789a94a18fb5be7db7769fd1121552d63f259fbd32b50c3dcf169cec0b02f978321db3bc60eb4b881b8327e9764f32e700236e0d8a35
|
|
Now that boost has been updated > 1.60, we can simplify how we get
wallet path relative to wallet_dir by using:
`boost::filesystem::lexically_relative`
|
|
|