faec689bed txmempool: Make entry time type-safe (std::chrono) (MarcoFalke)
faaa1f01da util: Add count_seconds time helper (MarcoFalke)
1111170f2f test: mempool entry time is persisted (MarcoFalke)
Pull request description:
This changes the type of the entry time of txs into the mempool from `int64_t` to `std::chrono::seconds`.
The benefits:
* Documents the type for developers
* Type violations result in compile errors
* After compilation, the two are equivalent (at no run time cost)
ACKs for top commit:
ajtowns:
utACK faec689bed
laanwj:
ACK faec689bed
Tree-SHA512: d958e058755d1a1d54cef536a8b30a11cc502b7df0d6ecf84a0ab1d38bc8105a67668a99cd5087a444f6de2421238111c5fca133cdf8e2e2273cb12cb6957845
1a02edb3f2 [RPC] Fix casing in getblockchaininfo to be inline with the rest of the response (Dan Gershony)
Pull request description:
The response in the RPC result `startTime` is camel cased while the rest of the response seems to be lower cased.
If this was intentional please ignore and close this PR.
Note: RPC field case changes might break existing callers
ACKs for top commit:
laanwj:
ACK 1a02edb3f2
Tree-SHA512: 6f0eaf2b4aaf73c9a9bf1fbd4af59af5f95fc012fa88f94e050e6ae273b3ad647f5729df53bfce91e1a925fe4fd7b14818908bb6131a81413a555137d1007d7c
The response in the RPC result `starttime` is camel cased while the rest of the response seems to be lower cased.
If this was intentional please ignore this PR.
Note: case might break existing callers
Reflect the change in the test data
Change to snake case
fa8d65f071 doc: Fix doxygen comment for SignTransaction in rpc/rawtransaction_util (MarcoFalke)
Pull request description:
The param `coins` to `SignTransaction` is final and can thus not be extended (as suggested by the doc).
ACKs for top commit:
practicalswift:
ACK fa8d65f071 -- const correctness is good and diff looks correct
fanquake:
ACK fa8d65f071
Tree-SHA512: 041e159f2c3cf96e296173c31f3e5f35bbc7711cc888aa4bf08aaa8c65c95ee7f7672f65396690a9af45795a618eea0fadde7fb02d29ec85f1b4df5e6d9e0c7a
2dfd6834ef test: Add test for default maxfeerate in sendrawtransaction (Joonmo Yang)
261843e4be wallet/rpc: Use the default maxfeerate value as BTC/kB (Joonmo Yang)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/16382
This patch tries to treat `maxfeerate` in sendrawtransaction/testmempoolaccept RPC as a rate(BTC/kB) instead of an absolute value(BTC).
The included test case checks if the new behavior works correctly, by using the transaction with an absolute fee of ~0.02BTC, where the fee rate is ~0.2BTC/kB.
This test should be failing if the default `maxfeerate` is 0.1BTC, but pass if the default value is 0.1BTC/kB
ACKs for top commit:
laanwj:
ACK 2dfd6834ef (ACKs by Sjors and MarcoFalke above for trivially different code)
Tree-SHA512: a1795bffe8a182acef8844797955db1f60bb0c0ded97148f3572dc265234d5219271a3a7aa0b6418a43f73b2b2720ef7412ba169c99bb1cdcac52051f537d6af
c0b5d97103 Test that joinpsbts randomly shuffles the inputs (Andrew Chow)
6f405a1d3b Shuffle inputs and outputs after joining psbts (Andrew Chow)
Pull request description:
`joinpsbts` currently just adds the inputs and outputs in the order of that the PSBTs were provided. This makes it extremely easy to identify which outputs belong to which inputs. This PR changes that so that all of the inputs and outputs are shuffled in the joined transaction.
ACKs for top commit:
instagibbs:
utACK c0b5d97103
jonatack:
ACK c0b5d97103 modulo suggestions for later.
Tree-SHA512: 14a0b7aae07d92e6d2c76a3a3b228b481e1964cb7d34f97515bdda18e2ea05a9f97c5a22affc143b86ae8b95c3cb239849fb54219d65512bc2112264dca915c8
This makes the RPC method consistent with other RPC methods that have a
'verbose' option.
Change the name of the return object from 'decoded' to details.
Update help text.
ec4c79326b signrawtransaction*: improve error for partial signing (Anthony Towns)
3c481f8921 signrawtransactionwithkey: better error messages for bad redeemScript/witnessScript (Anthony Towns)
Pull request description:
Two fixes for `signrawtransactionwith{key,wallet}` (in addition to #16250): one that checks redeemScript/witnessScript matches scriptPubKey (and if both are provided that they match each other sanely), and the other changes the warning when some-but-not-all the signatures for a CHECKMULTISIG are provided to something that suggests more signatures may be all that's required.
Fixes: #13218Fixes: #14823
ACKs for top commit:
instagibbs:
utACK ec4c79326b
achow101:
Code Review ACK ec4c79326b
meshcollider:
utACK ec4c79326b
Tree-SHA512: 0c95c91d498e85b834662b9e5c83f336ed5fd306be7701ce1dbfa0836fbeb448a267a796585512f7496e820be668b07c2a0a2f45e52dc23f09ee7d9c87e42b35
66740f460a doc: add a release note for the new field in 'getpeerinfo' and 'getnetworkinfo' (darosior)
6564f58c87 rpc/net: decode the services flags in a new entry (darosior)
Pull request description:
This is a reopen of https://github.com/bitcoin/bitcoin/pull/15511#issuecomment-527087370 since there have been concept ACKs from sdaftuar and Sjors.
This adds a new entry to `getpeerinfo` and `getnetworkinfo` which decodes the network services flags.
Here is a truncated output of `getpeerinfo`:
```
"services": "000000000000040d",
"servicesnames": "NODE_NETWORK | NODE_BLOOM | NODE_WITNESS | NODE_NETWORK_LIMITED",
"relaytxes": true,
```
And one of `getnetworkinfo`:
```
"localservices": "0000000000000409",
"localservicesnames": "NODE_NETWORK | NODE_WITNESS | NODE_NETWORK_LIMITED",
"localrelay": true,
```
Fixes#16780.
ACKs for top commit:
MarcoFalke:
unsigned ACK 66740f460a
laanwj:
ACK 66740f460a
Tree-SHA512: 0acc37134b283f56004a41243903d7790cb01591ddf0342489bd05f3a2c780563075373ba5fd55180fa15632e8968ffa11a979b8afece75a6a2e891342601440
This adds checks to ensure the redeemScript/witnessScript actually
correspond to the provided scriptPubKey, and, if both are provided,
that they are sensibly related to each other.
Thanks to github user passionofvc for raising this issue.
bdd6a4fd5d qa: Check scantxoutset result against gettxoutsetinfo (João Barbosa)
fc0c410d6e rpc: Improve scantxoutset response and help message (João Barbosa)
Pull request description:
The new response keys `height` and `bestblock` allow the client to know at what point the scan took place.
The help message now has all the response keys (`result` and `txouts` were missing) and it's improved a bit. Note that `searched_items` key is renamed to `txouts`, considering `scantxoutset` is marked experimental.
ACKs for top commit:
laanwj:
ACK bdd6a4fd5d
Tree-SHA512: 6bb7c3464b19857b756b8bc491ab7c58b0d948aad8c005b26ed27c55a1278f5639217e11a315bb505b4f44ebe86f413068c1e539c8a5f7a4007735586cc6443c
39034f1ee6 Refactor rawtransaction_util's SignTransaction to have previous tx parsing be separate (Andrew Chow)
Pull request description:
Currently the `SignTransaction` function has to handle both the actual signing and parsing of previous transaction data. This PR splits it so that `SignTransaction` only handles the signing itself and adds a `ParsePrevouts` function which handles parsing the prevtx information.
This allows for `SignTransaction` to just take any `SigningProvider`.
Split from #16341
ACKs for top commit:
MarcoFalke:
ACK 39034f1ee6
instagibbs:
utACK 39034f1ee6
ryanofsky:
utACK 39034f1ee6. No change since previously reviewed b49bbb939be92a67ff77c3f7bca5bb94dd141906, https://github.com/bitcoin/bitcoin/pull/16341#pullrequestreview-278610269 other than rebase with no conflicts.
Tree-SHA512: 09f7733e90691766bfb5cf0f20e913dbf270bd3b51abdcad966b24d110e562ed85fd3d0d1d7bbea61f903340060052ec73c4817b09aee0dc1f3916d781a9e40c
This adds a new boolean parameter 'decode' to the gettransaction call, which, if set to true, add a 'decoded' field to the result containing the decoded transaction
d48c1e837a Add window final block height to getchaintxstats (Jonathan "Duke" Leto)
Pull request description:
This patch is motivated by the desire to make the output of `getchaintxstats` more useful and optimized for applications to consume and render the data.
Firstly, this data is already available to the RPC, no additional work is done. Currently additional RPC calls will be needed to look up the height of the final block in the window or the block height that began the window.
By adding the block height of the final block in the window, the JSON is "self-contained" and applications can calculate the exact block height range of the window with no additional RPC requests.
For example, a web application which wants to render historical information for `getchaintxstats` RPC on various window sizes might call the RPC with various window lengths, once per day, and store the JSON results somewhere. Because the final block height of each dataset is included, it's no extra work to determine the exact block window range of each JSON response.
ACKs for top commit:
promag:
ACK d48c1e837a.
Tree-SHA512: fd4952c125f81a4ad18f7c78498c6b3e265b93cb574832166ac25596321ce84957f971f3f78f37d7e42638dc65f2a5d4d760f289873c9c2f2a82eb00a0f87c3f
These procedures will later be used in the ChainstateManager to compute
statistics (particularly a content hash) for UTXO sets coming in from
snapshots.
faebf62714 rpc: Use Join helper in rpc/util (MarcoFalke)
fa8cd6f9c1 util: Add Join helper to join a list of strings (MarcoFalke)
Pull request description:
We have a lot of enumerations in the code and sometimes those enumerations need to be mentioned in the RPC or command line documentation. Previously, each caller would have a couple of lines inline to join the strings or the joined string is hardcoded in the documentation. A helper to join strings would make code such as https://github.com/bitcoin/bitcoin/pull/16629#discussion_r315852446 less verbose and easier to read.
Also, warnings commonly accumulate in complex RPCs, since a warning doesn't lead to an early return. A helper to join those warnings would make code such as https://github.com/bitcoin/bitcoin/pull/16394/files#r309324997 less verbose and easier to read.
ACKs for top commit:
practicalswift:
ACK faebf62714
Tree-SHA512: 80f2db86a05c63b686f510585c1c631250271a8958fd71fafaac91559ffd2ec25d609bf7d53412ba27f87eff5893ac9dd9c2f296fc0c73581556e1d6a734a36f
Some failure conditions implicitly fail by failing some other check.
But the error messages are more helpful if they say explicitly what
actually caused the failure, so add those as failure conditions and
errors.
e78aaf41f4 [docs] Add release notes for burying bip 9 soft fork deployments (John Newbery)
8319e738f9 [tests] Add coverage for the content of getblockchaininfo.softforks (James O'Beirne)
0328dcdcfc [Consensus] Bury segwit deployment (John Newbery)
1c93b9b31c [Consensus] Bury CSV deployment height (John Newbery)
3862e473f0 [rpc] Tidy up reporting of buried and ongoing softforks (John Newbery)
Pull request description:
This hardcodes CSV and segwit activation heights, similar to the BIP 90 buried deployments for BIPs 34, 65 and 66.
CSV and segwit have been active for over 18 months. Hardcoding the activation height is a code simplification, makes it easier to understand segwit activation status, and reduces technical debt.
This was originally attempted by jl2012 in #11398 and again by me in #12360.
ACKs for top commit:
ajtowns:
ACK e78aaf41f4 ; checked diff to previous acked commit, checked tests still work
ariard:
ACK e78aaf4, check diff, run the tests again and successfully activated csv/segwit heights on mainnet as expected.
MarcoFalke:
ACK e78aaf41f4 (still didn't check if the mainnet block heights are correct, but the code looks good now)
Tree-SHA512: 7e951829106e21a81725f7d3e236eddbb59349189740907bb47e33f5dbf95c43753ac1231f47ae7bee85c8c81b2146afcdfdc11deb1503947f23093a9c399912
582d2cd747 Cover UTXO set access with lock annotations (James O'Beirne)
5693530685 refactor: have CCoins* data managed under CChainState (James O'Beirne)
fae6ab6aed refactor: pcoinsTip -> CChainState::CoinsTip() (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal
---
This change encapsulates UTXO set data within CChainState instances, removing global data `pcoinsTip` and `pcoinsviewdb`. This is necessary if we want to maintain multiple chainstates with their own rendering of the UTXO set.
We introduce a class CoinsViews which consolidates the construction of a CCoins* hierarchy.
This commit could be broken into smaller pieces, but it would require more ephemeral diffs to, e.g., temporarily change CCoinsViewDB's constructor invocations.
ACKs for top commit:
Sjors:
reACK 582d2cd747
MarcoFalke:
ACK 582d2cd747
Tree-SHA512: ec9d904fe5dca8cd2dc4b7916daa5d8bab30856dd4645987300f905e0a19f9919fce4f9d1ff03eda982943ca73e6e9a746be6cf53b46510de36e8c81a1eafba1
i.e. any CoinsViews members. Adds a lock acquisition to `gettxoutsetinfo` RPC
to comply with added annotations.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
This change encapsulates UTXO set data within CChainState instances, removing
global data `pcoinsTip` and `pcoinsviewdb`. This is necessary if we want to
maintain multiple chainstates with their own rendering of the UTXO set.
We introduce a class CoinsViews which consolidates the construction of a
CCoins* hierarchy. Construction of its various pieces (db, coinscatcher,
in-memory cache) is split up so that we avoid flushing bad state to disk if
startup is interrupted.
We also introduce `CChainState::CanFlushToDisk()` which tells us when it is
safe to flush the chainstate based on this partial construction.
This commit could be broken into smaller pieces, but it would require more
ephemeral diffs to, e.g., temporarily change CCoinsViewDB's constructor
invocations.
Other changes:
- A parameter has been added to the CCoinsViewDB constructor that allows the
name of the corresponding leveldb directory to be specified.
Thanks to Russell Yanofsky and Marco Falke for helpful feedback.
c5b404e8f1 Add functional tests for flexible whitebind/list (nicolas.dorier)
d541fa3918 Replace the use of fWhitelisted by permission checks (nicolas.dorier)
ecd5cf7ea4 Do not disconnect peer for asking mempool if it has NO_BAN permission (nicolas.dorier)
e5b26deaaa Make whitebind/whitelist permissions more flexible (nicolas.dorier)
Pull request description:
# Motivation
In 0.19, bloom filter will be disabled by default. I tried to make [a PR](https://github.com/bitcoin/bitcoin/pull/16176) to enable bloom filter for whitelisted peers regardless of `-peerbloomfilters`.
Bloom filter have non existent privacy and server can omit filter's matches. However, both problems are completely irrelevant when you connect to your own node. If you connect to your own node, bloom filters are the most bandwidth efficient way to synchronize your light client without the need of some middleware like Electrum.
It is also a superior alternative to BIP157 as it does not require to maintain an additional index and it would work well on pruned nodes.
When I attempted to allow bloom filters for whitelisted peer, my proposal has been NACKed in favor of [a more flexible approach](https://github.com/bitcoin/bitcoin/pull/16176#issuecomment-500762907) which should allow node operator to set fine grained permissions instead of a global `whitelisted` attribute.
Doing so will also make follow up idea very easy to implement in a backward compatible way.
# Implementation details
The PR propose a new format for `--white{list,bind}`. I added a way to specify permissions granted to inbound connection matching `white{list,bind}`.
The following permissions exists:
* ForceRelay
* Relay
* NoBan
* BloomFilter
* Mempool
Example:
* `-whitelist=bloomfilter@127.0.0.1/32`.
* `-whitebind=bloomfilter,relay,noban@127.0.0.1:10020`.
If no permissions are specified, `NoBan | Mempool` is assumed. (making this PR backward compatible)
When we receive an inbound connection, we calculate the effective permissions for this peer by fetching the permissions granted from `whitelist` and add to it the permissions granted from `whitebind`.
To keep backward compatibility, if no permissions are specified in `white{list,bind}` (e.g. `--whitelist=127.0.0.1`) then parameters `-whitelistforcerelay` and `-whiterelay` will add the permissions `ForceRelay` and `Relay` to the inbound node.
`-whitelistforcerelay` and `-whiterelay` are ignored if the permissions flags are explicitly set in `white{bind,list}`.
# Follow up idea
Based on this PR, other changes become quite easy to code in a trivially review-able, backward compatible way:
* Changing `connect` at rpc and config file level to understand the permissions flags.
* Changing the permissions of a peer at RPC level.
ACKs for top commit:
laanwj:
re-ACK c5b404e8f1
Tree-SHA512: adfefb373d09e68cae401247c8fc64034e305694cdef104bdcdacb9f1704277bd53b18f52a2427a5cffdbc77bda410d221aed252bc2ece698ffbb9cf1b830577
This combines reporting of buried (formally ISM) softfork deployments
and BIP9 versionbits softfork deployments into one JSON object in the
getblockchaininfo return object.
fb62f128bb Tidy up BroadcastTransaction() (John Newbery)
b8eecf8e79 Remove unused submitToMemoryPool and relayTransactions Chain interfaces (Antoine Riard)
8753f5652b Remove duplicate checks in SubmitMemoryPoolAndRelay (Antoine Riard)
611291c198 Introduce CWalletTx::SubmitMemoryPoolAndRelay (Antoine Riard)
8c8aa19b4b Add BroadcastTransaction utility usage in Chain interface (Antoine Riard)
Pull request description:
Remove CWalletTx::AcceptToMemoryPool
Replace CWalletTx::RelayWalletTransaction by SubmitMemoryPoolAndRelay
Add a relay flag to broadcastTransaction because wasn't sure of ReacceptWalletTransactions semantic.
Obviously, working on implementing https://github.com/bitcoin/bitcoin/pull/14978#issuecomment-459373984 to add the new higher-method in Node interface, will add a commit, just need more thought to do it cleanly
ACKs for top commit:
MarcoFalke:
re-ACK fb62f128bb
Sjors:
re-ACK fb62f128bb
Tree-SHA512: a7ee48b0545f537fa65cac8ed4cb24e777ab90b877d4eefb87971fa93c6a59bd555b62ad8940c6ffb40592a0bd50787d27587af99f20b56af72b415b6394251f
d6b3640ac7 [test] walletcreatefundedpsbt: check RBF is disabled when -walletrbf=0 (Sjors Provoost)
9ed062b568 [doc] rpc: remove "fallback to" from RBF default help (Sjors Provoost)
4fcb698bc2 [rpc] walletcreatefundedpsbt: use wallet default RBF (Sjors Provoost)
Pull request description:
The `walletcreatefundedpsbt` RPC call currently ignores `-walletrbf` and defaults to not use RBF. This PR fixes that.
This PR also replaces UniValue in `ConstructTransaction` with a `bool` in preparation of moving this helper method out of the RPC codebase entirely. This may be a bit overkill, but does slightly simplify it.
Fixes#15878
ACKs for top commit:
achow101:
Code Review ACK d6b3640ac7
l2a5b1:
re-ACK d6b3640
MarcoFalke:
ACK d6b3640ac7
Tree-SHA512: 55b9bccd1ef36b54f6b34793017dc0721103099ad3761b3b04862291ee13d6915915d4dbb1a8567924fa56e5e95dfe10eec070e06701610e70c87f8ea92b2a00
Harmonize the user-facing output of the `bitcoind -daemon`, `bitcoin-cli help stop`, `bitcoin-cli stop`, and `bitcoind -version` commands to be consistent with each other as well as with the "Bitcoin Core is probably already running" messages, e.g. `git grep 'probably already running.")'`.
Access through a broadcastTransaction method.
Add a wait_callback flag to turn off race protection when wallet
already track its tx being in mempool
Standardise highfee, absurdfee variable name to max_tx_fee
We drop the P2P check in BroadcastTransaction as g_connman is only
called by RPCs and the wallet scheduler, both of which are initialized
after g_connman is assigned and stopped before g_connman is reset.
682a1d0f20 refactoring: remove mapBlockIndex global (James O'Beirne)
55d525ab90 refactoring: make pindexBestInvalid internal to validation.cpp (James O'Beirne)
4ed55dfcd7 refactoring: add block_index_candidates arg to LoadBlockIndex (James O'Beirne)
613c46fe9e refactoring: move block metadata structures into BlockManager (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal
---
Under an assumeutxo model, we have multiple CChainState instances in use at once in order to support background validation. Currently, each CChainState instance has its own mapBlockIndex, a collection of linked block headers, in addition to a few other data structures that are related to maintenance of the block tree but not necessarily to any given chainstate.
In order to avoid duplicating this data across chainstates, this change moves chainstate-agnostic block metadata (and related behavior) into a class, `BlockManager`. Chainstates are parameterized with a reference to a blockmanager instance and in practice they share the same instance.
Most of this change is conceptually move-only, though the diff is somewhat muddled. The first commit can be reviewed slightly more easily with `--color-moved=dimmed_zebra`. Admittedly, that commit is pretty unwieldy; I tried to split it up after the fact with `git add --patch`, but that was difficult because of git's inability to split hunks past a certain point. Some of the moves also ended up being obscured when done over separate commits.
ACKs for top commit:
MarcoFalke:
ACK 682a1d0f20
ryanofsky:
utACK 682a1d0f20, only changes since last review were rebase and fixing conflict on a moved line
ariard:
utACK 682a1d0. Most of the changes are move-only, with main problem being to avoid creating circular dependencies between `BlockManager` and `CChainState`. Tested, comments are mostly nits, feel free to ignore them
Tree-SHA512: 738d8d06539ba53acf4bd2d48ae000473e645bbc4e63d798d55d247a4d5a4f781b73538ed590f6407be9ab402ea9d395570ea20bff0a4b9ce747bcc1600c5108
93ce4a0b6f Move WatchOnly stuff from SigningProvider to CWallet (Andrew Chow)
8f5b81e6ed Remove CCryptoKeyStore and move all of it's functionality into CWallet (Andrew Chow)
37a79a4fcc Move various SigningProviders to signingprovider.{cpp,h} (Andrew Chow)
16f8096e91 Move KeyOriginInfo to its own header file (Andrew Chow)
d9becff4e1 scripted-diff: rename CBasicKeyStore to FillableSigningProvider (Andrew Chow)
a913e3f2fb Move HaveKey static function from keystore to rpcwallet where it is used (Andrew Chow)
c7797ec655 Remove CKeyStore and squash into CBasicKeyStore (Andrew Chow)
1b699a5083 Add HaveKey and HaveCScript to SigningProvider (Andrew Chow)
Pull request description:
This PR compresses the `CWallet` chain of inheritance from 5 classes to 3 classes. `CBasicKeyStore` is renamed to `FillableSigningProvider` and some parts of it (the watchonly parts) are moved into `CWallet`. `CKeyStore` and `CCrypoKeyStore` are completely removed. `CKeyStore`'s `Have*` functions are moved into `SigningProvider` and the `Add*` moved into `FillableSigningProvider`, thus allowing it to go away entirely. `CCryptoKeyStore`'s functionality is moved into `CWallet`. The new inheritance chain is:
```
SigningProvider -> FillableSigningProvider -> CWallet
```
`SigningProvider` now is the class the provides keys and scripts and indicates whether keys and scripts are present. `FillableSigningProvider` allows keys and scripts to be added to the signing provider via `Add*` functions. `CWallet` handles all of the watchonly stuff (`AddWatchOnly`, `HaveWatchOnly`, `RemoveWatchOnly` which were previously in `CKeyStore`) and key encryption (previously in `CCryptoKeyStore`).
Implements the 2nd [prerequisite](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Class-Structure-Changes#cwallet-subclass-stack) from the wallet restructure.
ACKs for top commit:
Sjors:
re-ACK 93ce4a0; it keeps `EncryptSecret`, `DecryptSecret` and `DecryptKey` in `wallet/crypter.cpp`, but makes them not static. It improves alphabetical includes, reorders some function definitions, fixes commit message, brings back lost code comment.
instagibbs:
utACK 93ce4a0b6f
Tree-SHA512: 393dfd0623ad2dac38395eb89b862424318d6072f0b7083c92a0d207fd032c48b284f5f2cb13bc492f34557de350c5fee925da02e47daf011c5c6930a721b6d3
b6fb617aaa rpc: switch to using RPCHelpMan.Check() (Karl-Johan Alm)
c7a9fc234f Make the RPCHelpMan aware of JSONRPCRequest and add Check() helper (Karl-Johan Alm)
5c5e32bbe3 rpc: migrate JSONRPCRequest functionality into request.cpp (Karl-Johan Alm)
0ab8ba1ac6 rpc: fix RPC help requirements for getblocktemplate (Karl-Johan Alm)
Pull request description:
Every single RPC call has a helper-section at the start, which throws a help string if the user asks for help or if the user provided too few/many arguments.
```C++
const RPCHelpMan help{...};
if (request.fHelp || !help.IsValidNumArgs(request.params.size())) {
throw std::runtime_error(help.ToString());
}
```
or (older version)
```C++
if (request.fHelp || request.params.size() < min || request.params.size() > max)
throw std::runtime_error(
RPCHelpMan{...}.ToString()
);
```
It seems like an obvious improvement, and less copy-pasting, to make `RPCHelpMan` aware of `JSONRPCRequest`, and to let it handle the checks instead. Both of the above become
```C++
RPCHelpMan{...}.Check(request);
```
which means we save roughly 3 lines per RPC command, and the `RPCHelpMan` instance is never referenced afterwards, so the approach is a tiny fraction cleaner.
This is a complete update, sans a few special case locations that had special rules. 623 lines turn into 284 (which includes the addition to `RPCHelpMan`).
ACKs for top commit:
laanwj:
code rview and lightly tested ACK b6fb617aaa
MarcoFalke:
ACK b6fb617aaa, looked at the diff, verified move-only where applicable
Tree-SHA512: eb73f47f812512905b852e313281d1c8df803db40a6188aa39d5a7586631664db6764491152a8a96769946c796dc56d38c6e3a66ddd06ba3fb9d20050e6274e1
Moves all of the various SigningProviders out of sign.{cpp,h} and
keystore.{cpp,h}. As such, keystore.{cpp,h} is also removed.
Includes and the Makefile are updated to reflect this. Includes were largely
changed using:
git grep -l "keystore.h" | xargs sed -i -e 's;keystore.h;script/signingprovider.h;g'
c4606b8432 Add Travis check for single parameter constructors not marked "explicit" (practicalswift)
Pull request description:
Make single parameter constructors `explicit` (C++11).
Rationale from the developer notes:
> - By default, declare single-argument constructors `explicit`.
> - *Rationale*: This is a precaution to avoid unintended conversions that might
> arise when single-argument constructors are used as implicit conversion
> functions.
ACKs for top commit:
laanwj:
ACK c4606b8432
Tree-SHA512: 3e6fd51935fd93b2604b2188664692973d0897469f814cd745b5147d71b99ea5d73c1081cfde9f6393f51f56969e412fcda35d2d54e938a3235b8d40945f31fd
91cc18f602 [docs] Add release notes for PR 15427 (John Newbery)
3b11420b3c [RPC] add new utxoupdatepsbt arguments to the CRPCCommand and CPRCConvertParam tables (John Newbery)
Pull request description:
The new `descriptors` argument was not added to the CRPCCommand and CPRCCvertParam tables, meaning that it couldn't be used with bitcoin-cli or named arguments.
Before this PR:
```
> bitcoin-cli utxoupdatepsbt 'cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA==' "[{\"desc\":\"sh(wpkh([bd50871a/0h/0h/0h]03895c66337b38699bfafff1084ad35bc347fac4f4e5e5fe5eb7dd81155280db53))\"}]"
error code: -3
error message:
Expected type array, got string
> bitcoin-cli --named utxoupdatepsbt psbt='cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA==' descriptors="[{\"desc\":\"sh(wpkh([bd50871a/0h/0h/0h]03895c66337b38699bfafff1084ad35bc347fac4f4e5e5fe5eb7dd81155280db53))\"}]"
error code: -8
error message:
Unknown named parameter descriptors
```
After this PR:
```
bitcoin-cli utxoupdatepsbt 'cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA==' "[{\"desc\":\"sh(wpkh([bd50871a/0h/0h/0h]03895c66337b38699bfafff1084ad35bc347fac4f4e5e5fe5eb7dd81155280db53))\"}]"
cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA==
bitcoin-cli --named utxoupdatepsbt psbt='cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA==' descriptors="[{\"desc\":\"sh(wpkh([bd50871a/0h/0h/0h]03895c66337b38699bfafff1084ad35bc347fac4f4e5e5fe5eb7dd81155280db53))\"}]"
cHNidP8BAFMCAAAAAYCdwVRx2X3o4KHx5tAMsN1ddp51MbfWsietjfMbl5HtAAAAAAD/////AQDh9QUAAAAAF6kUW+rtEOi4nk9rpw2F5XZl1dd8ehGHAAAAAAAAAA==
```
ACKs for top commit:
promag:
ACK 91cc18f.
fanquake:
re-ACK 91cc18f602
Tree-SHA512: 279b2339a5cac17e363002e4ab743e251d6757c904c89f1970575bdce18d4f63d5e13507e171bf2bdc1bf6dd457db345a4b11b15d4ff71b96c2fedc4ffe52b23
The new `descriptors` argument needs to be added to the Command and
ConvertParams tables to by usable as a named argument and by
bitcoin-cli.
Also update the test to use named arguments to test this.
fab0c820fa rpc: Clarify that block count means height excl genesis (MarcoFalke)
Pull request description:
There is a common misconception that the block count returned by the blockchain rpcs includes the genesis block. See for example the discussion in https://github.com/bitcoin/bitcoin/pull/16292#issuecomment-506303256.
However, it really returns the height, which is `0` for the genesis block.
So clarify that and also remove the misleading "longest blockchain" comment.
Finally, fix the wallet test that incorrectly used this rpc.
ACKs for top commit:
instagibbs:
utACK fab0c820fa
promag:
ACK fab0c82, sorry for the misconception.
Tree-SHA512: 0d087cbb628d3866352bca6420402f392e6a997e579941701a408a7fca355d84645045661f39b022e4479cc07f85a6cddaa9095b6fd9911b245692482420a5e4
8a6810d0d2 Add a 'logpath' field to getrpcinfo (darosior)
Pull request description:
as discussed in #15438
ACKs for top commit:
laanwj:
Tested ACK 8a6810d0d2
Tree-SHA512: 752c7d90f670677c8144efb338c5c97c2264f85f1e65e031fd5a44f04230b6eafbabd0f634db263eb42c25642ecc1c4b1b602d4735e3fab07ec00b566134ddab
3b9bf0eb0e rpc: Allow shutdown while in generateblocks (Patrick Strateman)
Pull request description:
By checking the shutdown flag every loop we can use the entire 32 bit nonce space instead of breaking every 16 bits to check the flag.
This is possible now because the shutdown flag is an atomic where before it was controlled by a condition variable and lock.
ACKs for top commit:
kallewoof:
Re-ACK 3b9bf0e
Tree-SHA512: d0664201a55215130c2e9199a31fb81361daf4102a65cb3418984fd61cb98bfb9136d9ee8d23a85d57e50051f9bb0059bd71fe0488a17f63c38ea5caa6004504
01174596e6 signrawtransactionwithkey: report error when missing redeemScript/witnessScript param (Anthony Towns)
Pull request description:
Adding support for "witnessScript" as an alternative to "redeemScript" when using "signrawtransactionwithkey" meant that the `RPCTypeCheckObj()` call in `SignTransaction` can't error out just because either parameter is missing -- it's only a problem if both are missing, which isn't a state `RPCTypeCheckObj()` tests for. This results in the regression described in #16249. This patch adds some code to test for this case and give a similar error, namely:
error code: -8
error message:
Missing redeemScript/witnessScript
Fixes: #16249
ACKs for top commit:
meshcollider:
utACK 01174596e6
promag:
ACK 01174596e. Could also write test without `dict`/`del`:
Tree-SHA512: cf51346b7dea551b7f18f2a93c2a336a293b2535c62c03a5263cd2be8c58cf0cc302891da659c167e88ad1a68a756472c3c07e99f71627c61d32886fc5a3a353
26fe9b9909 Add support for descriptors to utxoupdatepsbt (Pieter Wuille)
3135c1a2d2 Abstract out UpdatePSBTOutput from FillPSBT (Pieter Wuille)
fb90ec3c33 Abstract out EvalDescriptorStringOrObject from scantxoutset (Pieter Wuille)
eaf4f88734 Abstract out IsSegWitOutput from utxoupdatepsbt (Pieter Wuille)
Pull request description:
This adds a descriptors argument to the `utxoupdatepsbt` RPC. This means:
* Input and output scripts and keys will be filled in when known.
* P2SH-witness inputs will be filled in from the UTXO set when a descriptor is provided that shows they're spending segwit outputs.
This also moves some (newly) shared code to separate functions: `UpdatePSBTOutput` (an analogue to `SignPSBTInput`), `IsSegWitOutput`, and `EvalDescriptorStringOrObject` (implementing the string or object notation parsing used in `scantxoutset`).
ACKs for top commit:
jnewbery:
utACK 26fe9b9909
laanwj:
utACK 26fe9b9909 (will hold merging until response to promag's comments)
promag:
ACK 26fe9b9, checked refactors and tests look comprehensive. Still missing a release note but can be added later.
Tree-SHA512: 1d833b7351b59d6c5ded6da399ff371a8a2a6ad04c0a8f90e6e46105dc737fa6f2740b1e5340280d59e01f42896c40b720c042f44417e38dfbee6477b894b245
fa2b083c3f [test] Add test to check mempool consistency in case of reorgs (MarcoFalke)
fabeb1f613 validation: Add missing mempool locks (MarcoFalke)
fa0c9dbf91 txpool: Make nTransactionsUpdated atomic (MarcoFalke)
Pull request description:
Take the mempool read lock during reorgs, so that we don't accidentally read an inconsistent mempool.
ACKs for top commit:
laanwj:
code review ACK fa2b083c3f
ryanofsky:
utACK fa2b083c3f [EDIT: was ~e284e422e75189794e24fe482819d8b1407857c3~, from bad copy and paste]. Changes since last review: rebase after #15976, adding vTxHashes lock annotation, adding new commit dropping mempool lock for nTransactionsUpdated and making it atomic to avoid deadlock between mempool lock and g_best_block_mutex
Tree-SHA512: cfe7777993589087753e000e3736d79d320dca412383fb77b56bef8946a04049722bf888c11b6f722adf677165185c7e58b4a269f7c5fa25e84dda375f6c8a7d
By checking the shutdown flag every loop we can use the entire nonce space
instead of breaking every 16 bits to check the shutdown flag.
This has been possible since the shutdown flag was switched to an atomic,
before that change it was controlled by a condition variable and lock.
a49503402b Make and get the multisig redeemscript and destination in one function instead of two (Andrew Chow)
Pull request description:
`CreateMultisigRedeemscript()` is changed to `AddAndGetMultisigDestination()` so that the process of constructing the redeemScript and then getting the `CTxDestination` are done in the same function. This allows that function to see what the keys in the multisig are so that the correct address type is returned from `AddAndGetDestinationForScript()`.
This only effects the `createmultisig` and `addmultisigaddress` RPCs and does not change signing logic as #16022 does.
Alternative to #16022 and #16012Fixes#16011
ACKs for commit a49503:
Tree-SHA512: 5b0154a714deea3b2cc3a54beb420c95eeeacf4ca30c40ca80940d9d640f8b03611b0fc14c2f0710bfd8a79e8d27ad7d9ae380b4b83d52b40ab201624f2a63f0
Instead of creating a redeemScript with CreateMultisigRedeemscript and
then getting the destination with AddAndGetDestinationForScript, do
both in the same function.
CreateMultisigRedeemscript is changed to AddAndGetMultisigDestination.
It creates the redeemScript and returns it via an output parameter. Then
it calls AddAndGetDestinationForScript to add the destination to the
keystore and get the proper destination.
This allows us to inspect the public keys in the redeemScript before creating
the destination so that the correct destination is used when uncompressed
pubkeys are in the multisig.
5ebc6b0eb2 bitcoind: update -avoidpartialspends description to account for auto-enable for avoid_reuse wallets (Karl-Johan Alm)
ada258f8c8 doc: release notes for avoid_reuse (Karl-Johan Alm)
27669551da wallet: enable avoid_partial_spends by default if avoid_reuse is set (Karl-Johan Alm)
8f2e208f7c test: add test for avoidreuse feature (Karl-Johan Alm)
0bdfbd34cf wallet/rpc: add 'avoid_reuse' option to RPC commands (Karl-Johan Alm)
f904723e0d wallet/rpc: add setwalletflag RPC and MUTABLE_WALLET_FLAGS (Karl-Johan Alm)
8247a0da3a wallet: enable avoid_reuse feature (Karl-Johan Alm)
eec15662fa wallet: avoid reuse flags (Karl-Johan Alm)
58928098c2 wallet: make IsWalletFlagSet() const (Karl-Johan Alm)
129a5bafd9 wallet: rename g_known_wallet_flags constant to KNOWN_WALLET_FLAGS (Karl-Johan Alm)
Pull request description:
Add a new wallet flag called `avoid_reuse` which, when enabled, will keep track of when a specific destination has been spent from, and will actively "blacklist" any new UTXOs which send to an already-spent-from destination.
This improves privacy, as a payer could otherwise begin tracking a payee's wallet by regularly peppering a known UTXO with dust outputs, which would then be scooped up and used in payments by the payee, allowing the payer to map out (1) the inputs owned by the payee and (2) the destinations to which the payee is making payments.
This replaces #10386 and together with the (now merged) #12257 it addresses #10065 in full. The concerns raised in https://github.com/bitcoin/bitcoin/pull/10386#issuecomment-302361381 are also addressed due to #12257.
~~Note: this builds on top of #15780.~~ (merged)
ACKs for commit 5ebc6b:
jnewbery:
ACK 5ebc6b0eb
laanwj:
Concept and code-review ACK 5ebc6b0eb2
meshcollider:
Code review ACK 5ebc6b0eb2
achow101:
ACK 5ebc6b0eb2 modulo above nits
Tree-SHA512: fdef45826af544cbbb45634ac367852cc467ec87081d86d08b53ca849e588617e9a0a255b7e7bb28692d15332de58d6c3d274ac003355220e4213d7d9070742e
fa499b5f02 rpc: bugfix: Properly use iswitness in converttopsbt (MarcoFalke)
fa5c5cd141 rpc: Switch touched RPCs to IsValidNumArgs (MarcoFalke)
Pull request description:
When a serialized transaction has inputs, there is no risk in only trying to deserialize it with witness allowed. (This is how all transactions from p2p are deserialized.) In fact, it would avoid a common issue where a transaction with inputs can be deserialized in two ways:
* Fixes#12989
* Fixes#15872
* Fixes#15701
* Fixes#13738
* ...
When a serialized transaction has no inputs, there is no risk in only trying to deserialze it with witness disallowed. (A transaction without inputs can't have corresponding witness data)
ACKs for commit fa499b:
meshcollider:
utACK fa499b5f02
ryanofsky:
utACK fa499b5f02. Changes since last review: consolidating commits and making iswitness documentation the same across methods.
PastaPastaPasta:
utACK fa499b5f02
Tree-SHA512: a64423a3131f3f0222a40da557c8b590c9ff01b45bcd40796f77a1a64ae74c6680a6be9d01ece95c492dfbcc7e2810409d2c2b336c2894af00bb213972fc85c6
c59e3a3261 getrawtransaction: inform about blockhash argument when lookup fails (darosior)
Pull request description:
Just 4 words added on `getrawtransaction` lookup error to fix#16142
ACKs for commit c59e3a:
Tree-SHA512: 2219099c1240667527a9b1498a58818b5ff1c2ef366c498d2bb57963e828b3c87fa3e6b94be7e6463bd289ceabc13f9c9b1082134641594ba335ac400e6d63aa
b748bf6f50 Fix spelling errors identified by codespell 1.15.0 (Ben Woosley)
Pull request description:
Note all changes are to comments / documentation.
After this commit, the only remaining output is:
```
$ test/lint/lint-spelling.sh
src/test/base32_tests.cpp:14: fo ==> of, for
src/test/base64_tests.cpp:14: fo ==> of, for
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/lint-spelling.ignore-words.txt
```
Note:
* I ignore several valid alternative spellings ~, but changed homogenous
to homogeneous as the latter is a more specific term according to the
Google dictionary definitions I found~
* homogenous is present in tinyformat, hence should be addressed upstream
* process' is correct only if there are plural processes
ACKs for commit b748bf:
practicalswift:
ACK b748bf6f50
fanquake:
ACK b748bf6f50
Tree-SHA512: 9add7044643ce015e0a44d8b27a3f300d72c485ffff550fb6491a17f14528085289ec5caddfe02f291ea9b2cded38a0dd3079652a054e2d7fe2ff4f7b53db5d7
After this commit, the only remaining output is:
$ test/lint/lint-spelling.sh
src/test/base32_tests.cpp:14: fo ==> of, for
src/test/base64_tests.cpp:14: fo ==> of, for
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/lint-spelling.ignore-words.txt
Note:
* I ignore several valid alternative spellings
* homogenous is present in tinyformat, hence should be addressed upstream
* process' is correct only if there are plural processes
67f4e9c522 Include core_io.h from core_read.cpp (practicalswift)
eca9767673 Make reasoning about dependencies easier by not including unused dependencies (practicalswift)
Pull request description:
Make reasoning about dependencies easier by not including unused dependencies.
Please note that the removed headers are _not_ "transitively included" by other still included headers. Thus the removals are real.
As an added bonus this change means less work for the preprocessor/compiler. At least 51 393 lines of code no longer needs to be processed:
```
$ git diff -u HEAD~1 | grep -E '^\-#include ' | cut -f2 -d"<" | cut -f1 -d">" | \
sed 's%^%src/%g' | xargs cat | wc -l
51393
```
Note that 51 393 is the lower bound: the real number is likely much higher when taking into account transitively included headers :-)
ACKs for commit 67f4e9:
Tree-SHA512: 0c8868aac59813f099ce53d5307eed7962dd6f2ff3546768ef9e5c4508b87f8210f1a22c7e826c3c06bebbf28bdbfcf1628ed354c2d0fdb9a31a42cefb8fdf13
403e677c9 refactoring: IsInitialBlockDownload -> CChainState (James O'Beirne)
3ccbc376d refactoring: FlushStateToDisk -> CChainState (James O'Beirne)
4d6688603 refactoring: introduce ChainstateActive() (James O'Beirne)
d7c97edee move-only: make the CChainState interface public (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal
---
This changeset starts moving functionality intimately related to CChainState into methods. Parameterizing these functions by a particular CChainState is necessary for the use of multiple chainstates simultaneously (e.g. for asynchronous background validation).
In this change, we
- make the CChainState interface public - since other units will start to invoke its methods directly,
- introduce `::ChainstateActive()`, the CChainState equivalent for `::ChainActive()`,
- and move `IsInitialBlockDownload()` and `FlushStateToDisk()` into methods on CChainState.
Independent of assumeutxo, these changes better encapsulate chainstate behavior and allow easier use from a testing context.
There are more methods that we'll move in the future, but they require other substantial changes (i.e. moving ownership of the `CCoinsView*` hierarchy into CChainState) so we'll save them for future PRs.
---
The first move-only commit is most easily reviewed with `git diff ... --color-moved=dimmed_zebra`.
ACKs for commit 403e67:
Empact:
utACK 403e677c9e no need to address my nits herein
Sjors:
utACK 403e677
ryanofsky:
utACK 403e677c9e. Only change since previous review is removing global state comment as suggested.
MarcoFalke:
utACK 403e677c9e, though the diff still seems a bit bloated with some unnecessary changes in the second commit.
promag:
utACK 403e677 and rebased with current [master](c7cfd20a7).
Tree-SHA512: 6fcf260bb2dc201361170c0b4547405366f5f331fcc3a2bac29b24442814b7b244ca1b58aac5af716885f9a130c343b544590dff780da0bf835c7c5b3ccb2257
e23809a05b [rpc] deriveaddresses: Correct descriptor checksum in RPCExamples (Chris Capobianco)
Pull request description:
Trvial: This fixes the descriptor checksum found the in the deriveaddresses RPC example.
The current checksum value does work, but only if the "h" used for the hardened derivation key origin are replaced with "'".
Given the discussion to switch from "'" to "h" [here](https://github.com/bitcoin/bitcoin/issues/15740), I thought it made more sense to update the checksum rather then changing all the "h" to "'" in this example.
ACKs for commit e23809:
instagibbs:
tACK e23809a05b
Tree-SHA512: 06a2b9f3e714ecde9b9a80b3b7a4082eb072e71d8abcc455ff5387e470d48839f22a70b78bbae1cf9122cb133fee46830819b6f39d67aec8c3c8d5889ae94e04
fa8ced32a6 doc: Mention blocksonly in reduce-traffic.md, unhide option (MarcoFalke)
fa320de79f test: Add test for p2p_blocksonly (MarcoFalke)
fa3872e7b4 test: Format predicate source as multiline on error (MarcoFalke)
fa1dce7329 net: Rename ::fRelayTxes to ::g_relay_txes (MarcoFalke)
Pull request description:
This is de-facto no longer hidden
ACKs for commit fa8ced:
jamesob:
utACK fa8ced32a6
Tree-SHA512: 474fbdee6cbd035ed9068a066b6056c1f909ec7520be0417820fcd1672ab3069b53f55c5147968978d9258fd3a3933fe1a9ef8e4f6e14fb6ebbd79701a0a1245
