| Age | Commit message (Collapse) | Author |
|---|
|
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`
|
|
d891ae768185b464cae476c16c74c365372d4a3c Introduce new V4 format addrman (Pieter Wuille)
Pull request description:
#23306 effectively changed the on-disk format in an incompatible way: old deserializers cannot deal with multiple entries for the same IP.
Introduce a `V4_MULTIPORT` format, and increment the compatibility base, so that old versions correctly recognize it as an incompatible future version, rather than corruption.
ACKs for top commit:
naumenkogs:
ACK d891ae768185b464cae476c16c74c365372d4a3c
ajtowns:
utACK d891ae768185b464cae476c16c74c365372d4a3c
vasild:
ACK d891ae768185b464cae476c16c74c365372d4a3c
Tree-SHA512: de2153beb59152504ee0656dd0cc0b879b09136eb07e3ce0426d2fea778adfabacebbce5cf1a9a65dc99ad4e99cda42ab26743fe672fb82a9fbfec49c4cccb4d
|
|
54011e7aa274bdc1b921440cc8b4623aa1e0d89e refactor: use CWallet const shared pointers when possible (Karl-Johan Alm)
96461989a2de737151bc4fb216221bf49cb53ce6 refactor: const shared_ptrs (Karl-Johan Alm)
Pull request description:
```C++
const std::shared_ptr<CWallet> wallet = x;
```
means we can not do `wallet = y`, but we can totally do `wallet->DestructiveOperation()`, contrary to what that line looks like.
This PR
* introduces a new convention: always use const shared pointers to `CWallet`s (even when we mutate the pointed-to thing)
* uses `const shared_ptr<const CWallet>` everywhere where wallets are not modified
In the future, this should preferably apply to all shared pointers, not limited to just `CWallet`s.
Both of these serve the same purpose: to dispell the misconception that `const shared_ptr<X>` immutates `X`. It doesn't, and it's dangerous to leave this misconception as is, for obvious reasons.
ACKs for top commit:
theStack:
re-ACK 54011e7aa274bdc1b921440cc8b4623aa1e0d89e
Tree-SHA512: 3bf4062fc821751be30770c6b4ead10a016847970f155a0a5156f304347d221b9830840030c2fbfba8cd1e282f4eda45f5b4107fe6df8138afdcb6c2e95a2836
|
|
Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>
|
|
This is not only cleaner but also helps make sure we are always using
the virtual size measure that includes the sigop weight heuristic (which
is the vsize the mempool would return).
|
|
This bool was originally part of Workspace and was removed in #22539
when it was no longer needed in Finalize(). Re-introducing it because,
once again, multiple functions will need to know whether we're doing an
RBF. Member of MemPoolAccept so that we can use this to inform package
RBF in the future.
|
|
We want to be able to re-use the precomputed transaction data between
PolicyScriptChecks and ConsensusScriptChecks in
AcceptMultipleTransactions.
|
|
No change in behavior.
ATMPArgs can continue to have granular rules like switching BIP125
on/off while we create an interface for the different sets of rules for
single transactions vs multiple-testmempoolaccept vs package validation.
This is a cleaner interface than manually constructing the args, which
makes it easy to mix up ordering, use the wrong default, etc. It also
means we don't need to edit ATMP/single transaction validation code
every time we update ATMPArgs for package validation.
|
|
Keep the internal {Function}_() functions grouped together.
Review with `git diff --color-moved=dimmed-zebra`
|
|
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.
p2p_addrv2_relay.py and p2p_addr_relay.py need to be updated since they
were incorrectly asserting on the buggy log (assuming that addresses are
added to addrman, when there could in fact be new table position
collisions that prevent some of those address records from being added).
|
|
|
|
Does not document the return value since we're going to fix the
semantics in a future commit.
|
|
bitcoin-wallet init implementations
d5f985e51f2863fda0c0d632e836eba42a5c3e15 multiprocess: Add new bitcoin-gui, bitcoin-qt, bitcoin-wallet init implementations (Russell Yanofsky)
Pull request description:
Add separate `interfaces::Init` subclasses for `bitcoin-wallet`, `bitcoin-gui`, and `bitcoin-qt` binaries instead of sharing `bitcoind` and `bitcoin-node` init subclasses in different binaries. After this, the new init subclasses can be customized in #10102, so node and wallet code is dropped from the `bitcoin-gui` binary and wallet code is dropped from into the `bitcoin-node` binary.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
ACKs for top commit:
lsilva01:
reACK d5f985e
hebasto:
re-ACK d5f985e51f2863fda0c0d632e836eba42a5c3e15, only suggested changes since my [previous](https://github.com/bitcoin/bitcoin/pull/23006#pullrequestreview-787537444) review.
Tree-SHA512: 6784210bd9ce3a6fbc66852680d0e9bc513c072dc538aeb7f48cb6a41580d3f567ccef04f975ee767a714a4b05d4d87273e94a79abda1b9c25d5ac4bbe752006
|
|
fa8fef6ef2287cd36ae14fbf10e852ddef7e62ac doc: Fix CWalletTx::Confirmation doc (MarcoFalke)
Pull request description:
Follow-up to:
* commit 700c42b85d20e624bef4228eef062c93084efab5, which replaced pIndex
with block_hash in AddToWalletIfInvolvingMe.
* commit 9700fcb47feca9d78e005b8d18b41148c8f6b25f, which replaced
posInBlock with confirm.nIndex.
ACKs for top commit:
laanwj:
Code review ACK fa8fef6ef2287cd36ae14fbf10e852ddef7e62ac
ryanofsky:
Code review ACK fa8fef6ef2287cd36ae14fbf10e852ddef7e62ac
Tree-SHA512: e7643b8e9200b8ebab3eda30435ad77006568a03476006013c9ac42c826c84e42e29bcdefc6f443fe4d55e76e903bfed5aa7a51f831665001c204634b6f06508
|
|
92617b7a758c0425330fba4b886296730567927c effectively changed the
on-disk format in an incompatible way: old deserializers cannot
deal with multiple entries for the same IP.
Introduce a V4_MULTIPORT format, and increment the compatibility base,
so that old versions correctly recognize it as an incompatible future
version.
|
|
92617b7a758c0425330fba4b886296730567927c Make AddrMan support multiple ports per IP (Pieter Wuille)
Pull request description:
For a long part of Bitcoin's history, this codebase has aggressively avoided making automatic connections to anything but nodes running on port 8333. I'd like to propose changing that, and this is a first PR necessary for that.
The folklore justification (eventually actually added as a comment to the codebase in #20668) is that this is to prevent the Bitcoin P2P network from being leveraged to perform a DoS attack on other services, if their IP/port would get rumoured. It appears, at least the current network scale - and probably significantly larger - that the impact is very low at best (see calculations by vasild in https://github.com/bitcoin/bitcoin/issues/5150#issuecomment-853888909 e.g.). Another possible justification would be a risk that treating different IP:port combinations separately would help perform Eclipse attacks (by an attacker rumouring their own IP with many ports). This concern is (a) no different than what is possible with IPv6 (where large ranges of IP addresses are very cheaply available), and (b) already hopefully sufficiently addressed by addrman's design (which limits access through based selected based on network groups).
And this policy has downsides too; in particular, a fixed port is easy to detect, and a very obvious sign a Bitcoin node is running there.
One obstacle in moving away from a default port that is the fact that addrman is currently restricted to a single entry per IP address. If ports are no longer expected to be generally always the default one, we need to deal with the case where conflicting information is relayed. It turns out there is a very natural solution to this: treat (IP,port) combination exactly as we're treating IPs now; this automatically means that the same IP may appear with multiple ports, simply because those would be distinct entries. Given that indexing into addrman's bucket _already_ uses the port number, the only change required is making all addrman lookup be (IP,port) (aka `CService`) based, rather than IP (aka `CNetAddr`) based.
This PR doesn't include any change to the actual outbound connection preference logic, as perhaps that's something that we want to phase in more gradually.
ACKs for top commit:
jnewbery:
Code review ACK 92617b7a758c0425330fba4b886296730567927c
naumenkogs:
ACK 92617b7a758c0425330fba4b886296730567927c
ajtowns:
ACK 92617b7a758c0425330fba4b886296730567927c
vasild:
ACK 92617b7a758c0425330fba4b886296730567927c
Tree-SHA512: 9eef06ce97a8b54a3f05fb8acf6941f253a9a5e0be8ce383dd05c44bb567cea243b74ee5667178e7497f6df2db93adab97ac66edbc37c883fd8ec840ee69a33f
|
|
This commit does not change behavior in any way. See previous commit for
complete rationale, but these flags are being disabled because they
aren't implemented and will otherwise break backwards compatibility when
they are implemented.
-BEGIN VERIFY SCRIPT-
sed -i 's:\(ALLOW_.*\) \(//!< unimplemented\):// \1\2:' src/util/system.h
sed -i '/DISALLOW_NEGATION.*scripted-diff/d' src/util/system.cpp
git grep -l 'ArgsManager::ALLOW_\(INT\|STRING\)' | xargs sed -i 's/ArgsManager::ALLOW_\(INT\|STRING\)/ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION/g'
git grep -l 'ALLOW_BOOL' -- ':!src/util/system.h' | xargs sed -i 's/ALLOW_BOOL/ALLOW_ANY/g'
-END VERIFY SCRIPT-
|
|
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 two commits, with this commit adding the DISALLOW_NEGATION flag,
and the next commit disabling the ALLOW_{INT|BOOL|STRING} flags.
|
|
Co-authored-by: Anthony Towns <aj@erisian.com.au>
|
|
da791c7f66243080177f92ce5f38c49305da63dc wallet: Use PACKAGE_NAME to mention our software (Hennadii Stepanov)
Pull request description:
This PR replaces "bitcoin" and "bitcoind" with `PACKAGE_NAME` in wallet log and error messages.
ACKs for top commit:
jonatack:
ACK da791c7f66243080177f92ce5f38c49305da63dc
lsilva01:
Tested ACK da791c7 on Ubuntu 20.04.
brunoerg:
tACK da791c7f66243080177f92ce5f38c49305da63dc
stratospher:
Tested ACK da791c7
shaavan:
ACK da791c7f66243080177f92ce5f38c49305da63dc
Tree-SHA512: c613446d9c8c3f85e6f5fec77382c9bc17746a853c89e72e1a3a79bf355d7bd9e455bbde8f9e02a894d225a67029c732cdc68ec8c58ac8237dde27d39dae8be7
|
|
performance of check() and remove dependency on validation
082c5bf099c64e3d27abe9b68a71ce500b693e7e [refactor] pass coinsview and height to check() (glozow)
ed6115f1eae0eb4669601106a9aaff078a2f3a74 [mempool] simplify some check() logic (glozow)
9e8d7ad5d9cc4b013826daead9cee09aad539401 [validation/mempool] use Spend/AddCoin instead of UpdateCoins (glozow)
09d18916afb0ecae90700d4befd9d5dc52767970 MOVEONLY: remove single-use helper func CheckInputsAndUpdateCoins (glozow)
e8639ec26aaf4de3fae280963434bf1cf2017b6f [mempool] remove now-unnecessary code (glozow)
54c6f3c1da01090aee9691a2c2bee0984a054ce8 [mempool] speed up check() by using coins cache and iterating in topo order (glozow)
30e240f65e69c6dffcd033afc63895345bd51f53 [bench] Benchmark CTxMemPool::check() (glozow)
cb1407196fba648aa75504e3ab3d46aa0181563a [refactor/bench] make mempool_stress bench reusable and parameterizable (glozow)
Pull request description:
Remove the txmempool <-> validation circular dependency by removing txmempool's dependency on validation. There are two functions in txmempool that need validation right now: `check()` and `removeForReorg()`. This PR removes the dependencies in `check()`.
This PR also improves the performance of `CTxMemPool::check()` by walking through the entries exactly once, in ascending ancestorcount order, which guarantees that we see parents before children.
ACKs for top commit:
jnewbery:
reACK 082c5bf099c64e3d27abe9b68a71ce500b693e7e
GeneFerneau:
tACK [082c5bf](https://github.com/bitcoin/bitcoin/pull/23157/commits/082c5bf099c64e3d27abe9b68a71ce500b693e7e)
rajarshimaitra:
tACK https://github.com/bitcoin/bitcoin/pull/23157/commits/082c5bf099c64e3d27abe9b68a71ce500b693e7e
theStack:
Code-review ACK 082c5bf099c64e3d27abe9b68a71ce500b693e7e
Tree-SHA512: 40ac622af1627b5c3e6abb4f0f035d833265a8c5e8dc88faf5354875dfb5137f137825e54bbd2a2668ed37b145c5d02285f776402629f58596e51853a9a79d29
|
|
a52f1d13409e4ef46277596ec13fa8b421fa1329 walletdb: Use SQLiteDatabase for mock wallet databases (Andrew Chow)
a78c2298080f173d0266e708267458a72eb2f600 tests: Place into mapWallet in coinselector_tests (Andrew Chow)
Pull request description:
#23288 changed coinselector_tests to use `DescriptorScriptPubKeyMan`, but it also ended up significantly slowing down the test, from 4 seconds to over 1 minute. It appears that the source of this slow down is with `CWallet::AddToWallet`, and primarily due to writing data to the mock wallet database. Because the only thing that is actually needed is for the created transaction to be placed into `CWallet::mapWallet`, this PR removes the call to `AddToWallet` and just places the transaction into `mapWallet` directly. This reduces the test time to 5 seconds.
To speed things up further, `CreateMockWalletDatabase` is changed to make a `SQLiteDatabase` instead of a `BerkeleyDatabase`. This is safe because there are no tests that require a specific mock database type.
ACKs for top commit:
brunoerg:
tACK a52f1d13409e4ef46277596ec13fa8b421fa1329
lsilva01:
tACK a52f1d1. Performed 74.36% better on Ubuntu 20.04 (VM, 12 MB, 8vCPU).
glozow:
utACK a52f1d13409e4ef46277596ec13fa8b421fa1329
Tree-SHA512: da77936bfd2e816d2e71703567b9389d0ee79f3a4a690802ffe3469df5bed371b296cb822b897f625654dab9436d91fd6bc02364a518a47d746e487d70a72595
|
|
While const shared_ptr<X> gives us an immutable shared pointer to a mutable X (we can't set it to some other X later), shared_ptr<const X> gives us a shared pointer to an immutable X. Importantly, we can recast shared_ptr<X> into shared_ptr<const X>, but not the other way around. We do this for two reasons: because it makes the code safer to guarantee the wallet is not modified, and because it further dispells the misconception that const shared_ptr<X> gives immutability to X.
|
|
Introduce convention to use const shared pointers everywhere, unless the shared pointer is modified at some point, which it very rarely is.
We want this convention, as it helps alleviate the misconception that a const shared pointer somehow results in a pointer to an immutable object, which is false.
|
|
|
|
Default to SQLiteDatabase instead of BerkeleyDatabase for
CreateDummyWalletDatabase. Most tests already use descriptor wallets and
the mock db doesn't really matter for tests. The tests where it does
matter will make the db directly.
|
|
Instead of using AddToWallet so that making a COutput will work,
directly add the transaction into wallet.mapWallet. This bypasses many
checks that AddToWallet will do which are pointless and just slow down
this test.
|
|
58765a450c40152db8160bca8a6b0f5b754c5858 qt: Use only Qt translation primitives in GUI code (W. J. van der Laan)
Pull request description:
Use `Object::tr`, `QT_TRANSLATE_NOOP`, and `QCoreApplication::translate` as appropriate instead of using `_()` which doesn't get picked up.
Replaces bitcoin/bitcoin#22764
Edit: I checked that the strings end up in the appropriate context in `bitcoin_en.ts` after `make translate`:
- "Settings file could not be read" "Settings file could not be written" end up in `bitcoin-core`
- "(press q to shutdown and continue later)" and "press q to shutdown" end up in `SplashScreen`
ACKs for top commit:
hebasto:
ACK 58765a450c40152db8160bca8a6b0f5b754c5858
Tree-SHA512: d0cc5901426c47d411d0fe75aac195b9684b51385f74c161da772dbf9f0977f7ad7a0976e17366f49b40718e9b6eb87c3e93306dc845f97adddbc47d00731742
|
|
For BnB, we only want to check that sets are equivalent with their
values, whereas in knapsack we care about the outpoints.
|
|
|
|
fa4ec1c0bdaef9f082a6661d7faf16149774e145 Make GenTxid boolean constructor private (MarcoFalke)
faeb9a575367119dbff60c35fa2c13547718e179 remove unused CTxMemPool::info(const uint256& txid) (MarcoFalke)
Pull request description:
This boolean argument is either verbose (when used with a named arg) or unintuitive and dangerous (when used as a plain bool).
Fix that by making the constructor private.
ACKs for top commit:
laanwj:
Code review ACK fa4ec1c0bdaef9f082a6661d7faf16149774e145
jnewbery:
Code review ACK fa4ec1c0bdaef9f082a6661d7faf16149774e145
glozow:
code review ACK fa4ec1c0bdaef9f082a6661d7faf16149774e145
Tree-SHA512: bf08ee09168885cfda71e5a01ec412b93964662a90dd9d91e75f7fdf2eaff7c21a95204d0e90b00438bfeab564d0aea66bdb9c0394ee7a05743e65a817159446
|
|
TransactionDescriptionString(), add coverage
66f6efc70a72cc1613906fd3c10281f9af0ba0db rpc: improve TransactionDescriptionString() "generated" help (Jon Atack)
296cfa312fd9ce19f1f820aeafa37d87764ad21d test: add listtransactions/listsinceblock "trusted" coverage (Jon Atack)
d95913fc432f0fde9dec743884b14c5df83727af rpc: fix "trusted" description in TransactionDescriptionString (Jon Atack)
Pull request description:
The RPC gettransaction, listtransactions, and listsinceblock helps returned by `TransactionDescriptionString()` inform the user that the `trusted` boolean field is only present if the transaction is trusted and safe to spend from.
The field is in fact returned by `WalletTxToJSON()` when the transaction has 0 confirmations (or negative confirmations, if conflicted), and it can be true or false.
This patch fixes the help, adds test coverage, and touches up the help for the neighboring `generate` field.
ACKs for top commit:
rajarshimaitra:
tACK https://github.com/bitcoin/bitcoin/pull/23139/commits/66f6efc70a72cc1613906fd3c10281f9af0ba0db
theStack:
Tested ACK 66f6efc70a72cc1613906fd3c10281f9af0ba0db
Tree-SHA512: 4c2127765b82780e07bbdbf519d27163d414d9f15598e01e02210f210e6009be344c84951d7274e747b1386991d4c3b082cd25aebe885fb8cf0b92d57178f68e
|
|
logic (stop on first match)
d047ed729f1d4732d23324fc76849f3c657cdbe4 external_signer: improve fingerprint matching logic (stop on first match) (Sebastian Falbesoner)
Pull request description:
The fingerprint matching logic in `ExternalSigner::SignTransaction` currently always iterates all inputs of a PSBT, even after a match has already been found. I guess the reason for that is not that it was not thought of, but rather the fact that breaking out of a nested loop is simply not possible (at least not without adding ugly constructs like gotos or extra state variables).
This PR fixes this by using `std::any_of` from C++'s standard library, see http://www.cplusplus.com/reference/algorithm/any_of/
ACKs for top commit:
lsilva01:
Code Review ACK https://github.com/bitcoin/bitcoin/pull/22789/commits/d047ed729f1d4732d23324fc76849f3c657cdbe4
Sjors:
utACK d047ed7
Zero-1729:
crACK d047ed729f1d4732d23324fc76849f3c657cdbe4
mjdietzx:
Code review ACK d047ed729f1d4732d23324fc76849f3c657cdbe4
hebasto:
ACK d047ed729f1d4732d23324fc76849f3c657cdbe4, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 447e7c0c6a5b5549a2c09d52e55ba4146302c1a06e4d96de11f6945d09f98c89129cba221202dff7e0718e01a83dd173b9f19b1f02b6be228978f3f6e35d8096
|
|
values under std::allocator
ea4b61a1570178ebe5851b5fb4065222e3926f7e refactor: remove references to deprecated values under std::allocator (Pasta)
Pull request description:
Removes usages of allocator::pointer, allocator::const_pointer, allocator::reference and allocator::const_reference which are deprecated in c++17 and **removed** in c++20. See https://en.cppreference.com/w/cpp/memory/allocator
Also prefers `using` over `typedef` see: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rt-using I'll be happy to revert this if requested so
ACKs for top commit:
laanwj:
Re-ACK ea4b61a1570178ebe5851b5fb4065222e3926f7e
Tree-SHA512: 9353e47a7de27bcd91b341eb2d832318b51fce9f508fcc791f05c02c5a160f430f4e7214e76f4b3e29639750d311c679789d8b7409255b13637391e4575c9ebe
|
|
from some wallet tests
2d2edc1248a2e49636409b07448676e5bfe44956 tests: Use Descriptor wallets for generic wallet tests (Andrew Chow)
99516285b7cf2664563712d95d95f54e1985c0c2 tests: Use legacy change type in subtract fee from outputs test (Andrew Chow)
dcd6eeb64adb2b532f5003cbb86ba65b3c08a87b tests: Use descriptors in psbt_wallet_tests (Andrew Chow)
4b1588c6bd96743b333cc291e19a9fc76dc8cdf1 tests: Use DescriptorScriptPubKeyMan in coinselector_tests (Andrew Chow)
811319fea4295bfff05c23c0dcab1e24c85e8544 tests, gui: Use DescriptorScriptPubKeyMan in GUI tests (Andrew Chow)
9bf02438727e1052c69d906252fc2a451c923409 bench: Use DescriptorScriptPubKeyMan for wallet things (Andrew Chow)
5e54aa9b90c5d4d472be47a7fca969c5e7b92e88 bench: remove global testWallet from CoinSelection benchmark (Andrew Chow)
a5595b1320d0ebd2c60833286799ee42108a7c01 tests: Remove global vCoins and testWallet from coinselector_tests (Andrew Chow)
Pull request description:
Currently, various tests use `LegacyScriptPubKeyMan` because it was convenient for the refactor that introduced the `ScriptPubKeyMan` interface. However, with the legacy wallet slated to be removed, these tests should not continue to use `LegacyScriptPubKeyMan` as they are not testing any specific legacy wallet behavior. These tests are changed to use `DescriptorScriptPubKeyMan`s.
Some of the coin selection tests and benchmarks had a global `testWallet`, but this seemed to cause some issues with ensuring that descriptors were set up in that wallet for each test. Those have been restructured to not have any global variables that may be modified between tests.
The tests which test specific legacy wallet behavior remain unchanged.
ACKs for top commit:
laanwj:
Code review ACK 2d2edc1248a2e49636409b07448676e5bfe44956
brunoerg:
tACK 2d2edc1248a2e49636409b07448676e5bfe44956
Tree-SHA512: 6d60e5978e822d48e46cfc0dae4635fcb1939f21ea9d84eb72e36112e925554b7ee8f932c7ed0c4881b6566c6c19260bec346abdff1956ca9f300b30fb4e2dd1
|
|
default-ctored `external_spk_managers` entry
6911ab95f19d2b1f60f2d0b2f3961fa6639d4f31 wallet: fix segfault by avoiding invalid default-ctored `external_spk_managers` entry (Sebastian Falbesoner)
Pull request description:
Fixes #23321 (bug reported by Josef Vondrlik (josef-v)).
In the method `CWallet::LoadActiveScriptPubKeyMan`, the map `external_spk_managers` (or `internal_spk_managers`, if parameter `internal` is false) is accessed via std::map::operator[], which means that a default-ctored entry is created with a null-pointer as value, if the key doesn't exist. As soon as this value is dereferenced, a segmentation fault occurs, e.g. in `CWallet::KeypoolCountExternalKeys`.
The bevaviour can be reproduced by the following steps (starting with empty regtest datadir):
```
$ ./src/bitcoind -regtest -daemon
$ ./src/bitcoin-cli -regtest -named createwallet_name=wallet descriptors=true blank=true
$ cat regtest-descriptors.txt
[
{
"desc": "tr([e4445899/49'/1'/0']tprv8ZgxMBicQKsPd8jCeBWsYLEoWxbVgzJDatJ7XkwQ6G3uF4FsHuaziHQ5JZAW4K515nj6kVVwPaNWZSMEcR7aFCwL4tQqTcaoprMKTTtm6Zg/1/*)#mr3llm7f",
"timestamp": 1634652324,
"active": true,
"internal": true,
"range": [
0,
999
],
"next": 0
}
]
$ ./src/bitcoin-cli -regtest importdescriptors "$(cat regtest-descriptors.txt)"
[
{
"success": true
}
]
$ ./src/bitcoin-cli -regtest getwalletinfo
error: timeout on transient error: Could not connect to the server 127.0.0.1:18443 (error code 1 - "EOF reached")
```
ACKs for top commit:
achow101:
Code Review ACK 6911ab95f19d2b1f60f2d0b2f3961fa6639d4f31
lsilva01:
Tested ACK 6911ab9 on Ubuntu 20.04.
instagibbs:
ACK 6911ab95f19d2b1f60f2d0b2f3961fa6639d4f31
Tree-SHA512: 76aa96847cf2739413fb68fb902afef0b3ab9381178dd62fb0abac69f853f1f6523d73c60e610375b9a7730f275eda9162503b89f5be6e6e349a8d047b59c8dc
|
|
positions, first
632aad9e6d8369750f4327a886ca5b3d3fed89bd Make CAddrman::Select_ select buckets, not positions, first (Pieter Wuille)
Pull request description:
The original CAddrMan behaviour (before #5941) was to pick a uniformly random non-empty bucket, and then pick a random element from that bucket. That commit, which introduced deterministic placement of entries in buckets, changed this to picking a uniformly random non-empty bucket position instead.
I believe that was a mistake. Buckets are our best metric for spreading out requests across independently-controlled nodes. That
does mean that if a bucket has fewer entries, its entries are supposed to be picked more frequently.
This PR reverts to the original high-level behavior, but on top of the deterministic placement logic.
ACKs for top commit:
jnewbery:
utACK 632aad9e6d8369750f4327a886ca5b3d3fed89bd
naumenkogs:
ACK 632aad9e6d8369750f4327a886ca5b3d3fed89bd
mzumsande:
ACK 632aad9e6d8369750f4327a886ca5b3d3fed89bd
Tree-SHA512: 60768afba2b6f0abd0dddff04381cab5acf374df48fc0e883ee16dde7cf7fd33056a04b573cff24a1b4d8e2a645bf0f0b3689eec84da4ff330e7b59ef142eca1
|
|
9c1052a5218e191fd23c0d9fc06f2fca34b03411 wallet: Default new wallets to descriptor wallets (Andrew Chow)
f19ad404631010a5e2dac2c7cbecd057b005fe2a rpc, wallet: Descriptor wallets are no longer experimental (Andrew Chow)
Pull request description:
Changes the default wallet type from legacy to descriptors. Descriptor wallets will now by the default type. Additionally, descriptor wallets will no longer be marked as experimental.
This follows the timeline proposed in #20160
ACKs for top commit:
lsilva01:
Tested ACK https://github.com/bitcoin/bitcoin/pull/23002/commits/9c1052a5218e191fd23c0d9fc06f2fca34b03411 on Ubuntu 20.04
prayank23:
tACK https://github.com/bitcoin/bitcoin/pull/23002/commits/9c1052a5218e191fd23c0d9fc06f2fca34b03411
meshcollider:
Code review ACK 9c1052a5218e191fd23c0d9fc06f2fca34b03411
Tree-SHA512: 834e6fec88e0c18673af7ebe135bd5333694d1be502164eb93a90e3e76c27974165aa4e59426945100c88e4eca07356e16886ef5b05cf789683ecb23fc71a12a
|
|
disconnecting misbehaving peer
fa2662c293ec0aaa93092b59b6632f74729c4283 net: Avoid logging AlreadyHaveTx when disconnecting misbehaving peer (MarcoFalke)
Pull request description:
There is no need to log `AlreadyHaveTx` for an inv when a peer is marked for disconnection due to sending that inv. In fact, I find it confusing that a `block-relay-only` connection calls `AlreadyHaveTx` at all. Also there is no need to call `AddKnownTx` when the peer is marked for disconnection.
ACKs for top commit:
naumenkogs:
ACK fa2662c293ec0aaa93092b59b6632f74729c4283
jnewbery:
Code review ACK fa2662c293ec0aaa93092b59b6632f74729c4283
dunxen:
Concept and code review ACK fa2662c293ec0aaa93092b59b6632f74729c4283
Tree-SHA512: 9996b807a824021f992b5281d82ff0cbbe6a442c2fedf7dfd6adda64ccc5e0ef4fb0ff91ab75086f975837bbbb7a5934ac7e671a80dcababa7203c92fc0c7f84
|
|
|
|
Can be reviewed with --word-diff-regex=.
|
|
|
|
|
|
4307849256761fe2440d82bbec892d0e8e6b4dd4 [mempool] delete exists(uint256) function (glozow)
d50fbd4c5b4bc72415854d582cedf94541a46983 create explicit GenTxid::{Txid, Wtxid} ctors (glozow)
Pull request description:
We use the same type for txids and wtxids, `uint256`. In places where we want the ability to pass either one, we distinguish them using `GenTxid`.
The (overloaded) `CTxMemPool::exists()` function is defined as follows:
```c
bool exists(const uint256& txid) const { return exists(GenTxid{false, txid}); }
```
It always assumes that a uint256 is a txid, which is a footgunny interface.
Querying by wtxid returns a false negative if the transaction has a witness. :bug:
Another approach would be to try both:
```c
bool exists(const uint256& txid) const { return exists(GenTxid{false, txid}) || exists(GenTxid{false, txid}); }
```
But that's slower and wrongfully placing the burden on the callee; the caller always knows whether the hash is a txid or a wtxid.
ACKs for top commit:
laanwj:
Code review ACK 4307849256761fe2440d82bbec892d0e8e6b4dd4
jnewbery:
Tested and code review ACK 4307849256761fe2440d82bbec892d0e8e6b4dd4
MarcoFalke:
review ACK 4307849256761fe2440d82bbec892d0e8e6b4dd4 👘
Tree-SHA512: 8ed167a96f3124b6c14e41073c8358658114ce121a15a4cca2db7a5ac565903a6236e34e88ac03382b8bb8b68e3999abbfc5718bc8c22476554d6b49a5298eec
|
|
... needed for std::numeric_limits<T>::max on WIN32
|
|
Follow-up to:
* commit 700c42b85d20e624bef4228eef062c93084efab5, which replaced pIndex
with block_hash in AddToWalletIfInvolvingMe.
* commit 9700fcb47feca9d78e005b8d18b41148c8f6b25f, which replaced
posInBlock with confirm.nIndex.
|
|
1946af2c45372e3de39000a45a5954bb5870bc1b Add comment to COIN constant. (Kennan Mell)
Pull request description:
The COIN constant is critical in understanding Bitcoin's supply, but what it represents isn't clear from the name of the constant. Adding a comment clarifies the meaning of the constant for future readers.
ACKs for top commit:
lsilva01:
ACK 1946af2
shaavan:
ACK 1946af2c45372e3de39000a45a5954bb5870bc1b
Tree-SHA512: ba27c6bd4a4c92664a71ef081104e9e50e47022d68eb955c247b7c49a0046a42bf64bf23a14563c646411b7be6027fea918cc62baa01bbf319b82d14d368f280
|
