faa24441ec policy: Remove promiscuousmempoolflags (MarcoFalke)
Pull request description:
It seems odd to clutter validation code with features that can only ever be used for testing (testnet or regtest). Removing that test-only code makes the mempool logic less painful to understand and easier to reason about when changed or refactored in the future.
Tree-SHA512: 3b897aa9604ac8d82ebe9573c6efd468c93ddaa08d378ebc902e247b7aa6c68fcde71e5b449c08f17a067146cdc66dc50a67ce06d07607c27e5189a49c3fba3f
This linter checks that the number of arguments passed to each variadic format
string function matches the number of format specifiers in the format string.
a1a998cf24 wallet: Fix backupwallet for multiwallets (Daniel Kraft)
Pull request description:
`backupwallet` was broken for multiwallets in their own directories (i.e. something like `DATADIR/wallets/mywallet/wallet.dat`). In this case, the backup would use `DATADIR/wallets/wallet.dat` as source file and not take the specific wallet's directory into account.
This led to either an error during the backup (if the wrong source file was not present) or would silently back up the wrong wallet; especially the latter behaviour can be quite bad for users.
Tree-SHA512: 7efe2450ca047e40719fcc7cc211ed94699056020ac737cada7b59e8240298675960570c45079add424d0aab520437d5050d956acd695a9c2452dd4317b4d2c4
fa67505e1e qa: Quote wallet name for rpc path (MarcoFalke)
Pull request description:
When using external multiwallets they are specified by their full path which might contain non-ascii characters (e.g. umlauts or emojis).
Fix this by url-quoting the path.
Tree-SHA512: 7cc66514579d9f602f88a6817c5ab43a44c6d3711df452dc904173f0bc34e2c0b2c34877497f06b61f6720c532fa183053f54622dc454e316c89cee7eaa72463
fa5b440971 qa: Extract rpc_timewait as test param (MarcoFalke)
Pull request description:
Also increase it for wallet_dump and wallet_groups
Tree-SHA512: 7367bc584228bda3010c453713a1505c54a8ef3d116be47dab9934d30594089dfeb27ffa862f7517fd0ec8b5dc07f4904d67ef2a53dd284cbe2a58982e410e2b
f6b7fc349c Support h instead of ' in hardened descriptor paths (Pieter Wuille)
fddea672eb Add experimental warning to scantxoutset (Jonas Schnelli)
6495849bfd [QA] Extend tests to more combinations (Pieter Wuille)
1af237faef [QA] Add xpub range tests in scantxoutset tests (Jonas Schnelli)
151600bb49 Swap in descriptors support into scantxoutset (Pieter Wuille)
0652c3284f Descriptor tests (Pieter Wuille)
fe8a7dcd78 Output descriptors module (Pieter Wuille)
e54d76044b Add simple FlatSigningProvider (Pieter Wuille)
29943a904a Add more methods to Span class (Pieter Wuille)
Pull request description:
As promised, here is an implementation of my output descriptor concept (https://gist.github.com/sipa/e3d23d498c430bb601c5bca83523fa82) and integration within the `scantxoutset` RPC that was just added through #12196.
It changes the RPC to use descriptors for everything; I hope the interface is simple enough to encompass all use cases. It includes support for P2PK, P2PKH, P2WPKH, P2SH, P2WSH, multisig, xpubs, xprvs, and chains of keys - combined in every possible way.
Tree-SHA512: 63b54a96e7a72f5b04a8d645b8517d43ecd6a65a41f9f4e593931ce725a8845ab0baa1e9db6a7243190d8ac841f6e7e2f520d98c539312d78f7fd687d2c7b88f
a13647b8bd [qa] Add test for too-large wallet output groups (Suhas Daftuar)
57ec1c97b2 [wallet] correctly limit output group size (Suhas Daftuar)
Pull request description:
Also add a test to ensure that output groups are being limited, even if a wallet has many outputs corresponding to the same scriptPubKey (the test fails without the first commit).
Tree-SHA512: 2aaa82005b0910488f5cbf40690d4c5e2f46949e299ef70b4cb6e440713811443d411dcbc6d71b1701fd82423073125e21747787d70830cd021c841afb732d51
fad231ad41 Fix merging of global unknown data in PSBTs (Andrew Chow)
41df035ee1 Check that PSBT keys are the correct length (Andrew Chow)
Pull request description:
This PR fixes a few bugs that were found and adds tests checking for these errors.
Specifically:
- Single byte keys are checked to actually be one byte.
- Unknown global data must be merged when combining two PSBTs.
Tree-SHA512: c0e7b4bc607d510cc005aaa7c0813ee58c5467ab7ce4adce485522dfeee92b1af3d29fe89df778b0ea812bb3827e085b30e04d4f4ebcefd8364d809573991332
12dd101345 scripted-diff: Remove trailing whitespaces (João Barbosa)
Pull request description:
The script test/lint/lint-whitespace.sh should prevent new cases.
This happens in some pulls where the code editor and the author 'git add's them, so this would fix it all.
Tree-SHA512: bcdd3472fcd01a2754e52212c7db1de2fdc422728b06785481954a27162fb72001cb73708329cc56e95bcc5e45c1348ebc4eacc2ccfa6aa12413c7ec450b6a33
e3245f2e7b Removes Boost predicate.hpp dependency (251)
Pull request description:
This pull request removes the `boost/algorithm/string/predicate.hpp` dependency from the project.
To replace the the `predicate.hpp` dependency from the project the function calls to `boost::algorithm::starts_with` and `boost::algorithm::ends_with` have been replaced with respectively C++11's `std::basic_string::front` and `std::basic_string::back` function calls.
Refactors that were not required, but have been done anyways:
- The Boost function `all` was implicitly made available via the `predicate.hpp` header. Instead of including the appropriate header, function calls to `all` have been replaced with function calls to `std::all_of`.
- The `boost::algorithm::is_digit` predicate has been replaced with a custom `IsDigit` function that is locale independent and ASCII deterministic.
Tree-SHA512: 22dda6adfb4d7ac0cabac8cc33e8fb8330c899805acc1ae4ede402c4b11ea75a399414b389dfaa3650d23b47f41351b4650077af9005d598fbe48d5277bdc320
232f96f5c8 doc: Add release notes for -avoidpartialspends (Karl-Johan Alm)
e00b4699cc clean-up: Remove no longer used ivars from CInputCoin (Karl-Johan Alm)
43e04d13b1 wallet: Remove deprecated OutputEligibleForSpending (Karl-Johan Alm)
0128121101 test: Add basic testing for wallet groups (Karl-Johan Alm)
59d6f7b4e2 wallet: Switch to using output groups instead of coins in coin selection (Karl-Johan Alm)
87ebce25d6 wallet: Add output grouping (Karl-Johan Alm)
bb629cb9dc Add -avoidpartialspends and m_avoid_partial_spends (Karl-Johan Alm)
65b3eda458 wallet: Add input bytes to CInputCoin (Karl-Johan Alm)
a443d7a0ca moveonly: CoinElegibilityFilter into coinselection.h (Karl-Johan Alm)
173e18a289 utils: Add insert() convenience templates (Karl-Johan Alm)
Pull request description:
This PR adds an optional (off by default) `-avoidpartialspends` flag, which changes coin select to use output groups rather than outputs, where each output group corresponds to all outputs with the same destination.
It is a privacy improvement, as each time you spend some output, any other output that is publicly associated with the destination (address) will also be spent at the same time, at the cost of fee increase for cases where coin select without group restriction would find a more optimal set of coins (see example below).
For regular use without address reuse, this PR should have no effect on the user experience whatsoever; it only affects users who, for some reason, have multiple outputs with the same destination (i.e. address reuse).
Nodes with this turned off will still try to avoid partial spending, if the fee of the resulting transaction is not greater than the fee of the original transaction.
Example: a node has four outputs linked to two addresses `A` and `B`:
* 1.0 btc to `A`
* 0.5 btc to `A`
* 1.0 btc to `B`
* 0.5 btc to `B`
The node sends 0.2 btc to `C`. Without `-avoidpartialspends`, the following coin selection will occur:
* 0.5 btc to `A` or `B` is picked
* 0.2 btc is output to `C`
* 0.3 - fee is output to (unique change address)
With `-avoidpartialspends`, the following will instead happen:
* Both of (0.5, 1.0) btc to `A` or `B` is picked (one or the other pair)
* 0.2 btc is output to `C`
* 1.3 - fee is output to (unique change address)
As noted, the pro here is that, assuming nobody sends to the address after you spend from it, you will only ever use one address once. The con is that the transaction becomes slightly larger in this case, because it is overpicking outputs to adhere to the no partial spending rule.
This complements #10386, in particular it addresses @luke-jr and @gmaxwell's concerns in https://github.com/bitcoin/bitcoin/pull/10386#issuecomment-300667926 and https://github.com/bitcoin/bitcoin/pull/10386#issuecomment-302361381.
Together with `-avoidreuse`, this fully addresses the concerns in #10065 I believe.
Tree-SHA512: 24687a4490ba59cf4198ed90052944ff4996653a4257833bb52ed24d058b3e924800c9b3790aeb6be6385b653b49e304453e5d7ff960e64c682fc23bfc447621
https://github.com/bitcoin/bitcoin/pull/13715 introduced a new check
for _transport.is_closing() in mininode's P2PConnection's. This function
is only available from Python 3.4.4, though, while Bitcoin is supposed
to support all Python 3.4 versions.
In this change, we make the check conditional on is_closing() being
available. If it is not, then we revert to the behaviour before the
check was introduced; this means that
https://github.com/bitcoin/bitcoin/issues/13579 is not fixed for old
systems, but at least the tests work as they used to do before.
This includes a small refactoring from a one-line lambda to an
inline function, because this makes the code easier to read with more
and more conditions being added.
Fixes https://github.com/bitcoin/bitcoin/issues/13745.
This is a squashed commit that squashes the following commits:
This commit removes the `boost/algorithm/string/predicate.hpp` dependenc
from the project by replacing the function calls to `boost::algorithm::starts_with`
`boost::algorithm::ends_with` and `all` with respectively C++11'
`std::basic_string::front`, `std::basic_string::back`, `std::all_of` function calls
This commit replaces `boost::algorithm::is_digit` with a locale independent isdigi
function, because the use of the standard library's `isdigit` and `std::isdigit
functions is discoraged in the developer notes
5f019d5354 Removes the boost/algorithm/string/join dependency (251)
Pull request description:
This commit removes the `boost/algorithm/string/join` dependency from the project by replacing `boost::algorithm::join` with the helper function proposed by @MarcoFalke in https://github.com/bitcoin/bitcoin/pull/13726#discussion_r204159967
Tree-SHA512: d4ba3e7621b76bd5210aec9b8d6c320f7ee963d7f902e6d2d3fc0eadbee1cd77799e5c09be9c11452d2825f25740fc436cdec3a6b6c66ced674d771e4ed306ae
f447a0a707 Remove program options from build system (Chun Kuan Lee)
11588c639e Replace boost program_options (Chun Kuan Lee)
Pull request description:
Concept from #12744, but without parsing negated options.
Tree-SHA512: 7f418744bb8934e313d77a5f162633746ef5d043de802b9c9cd9f7c1842e7e566eb5f171cd9e2cc13317281b2449c6fbd553fa4f09b837e6af2f5d2b2aabdca2
5c613aadd6 lint: Add linter for circular dependencies (Ben Woosley)
Pull request description:
Protects against added circular depencies, makes it explicit in the
code when circular dependencies have been removed.
Modeled after EXPECTED_BOOST_INCLUDES in lint-includes.sh
Example output:
```
$ test/lint/lint-circular-dependencies.sh
A new circular dependency in the form of "qt/paymentserver -> qt/walletmodel -> qt/paymentserver" appears to have been introduced.
$ echo $?
1
$ test/lint/lint-circular-dependencies.sh
Good job! The circular dependency "Fake" is no longer present.
Please remove it from EXPECTED_CIRCULAR_DEPENDENCIES in test/lint/lint-circular-dependencies.sh
to make sure this circular dependency is not accidentally reintroduced.
$ echo $?
1
$ test/lint/lint-circular-dependencies.sh
$ echo $?
0
```
Tree-SHA512: 4519434de29f6d50859daed1480e531c01c1cdbc3f0a5f093251daf62ae2b5b9073fb274b86f541a985e06837aa1165b76558c5f35fb51a759d72e83f1b61e44
fa4bf92be9 Remove dead service bits code (MarcoFalke)
Pull request description:
Seems fine to remove for the upcoming 0.17 release
Fixes#10993
Tree-SHA512: 3a4664b787e3da399bcaaba693619bd384826df14f469dbdfbbfffc540d9da3f2b322cda262b43388376785f77907c2540541c239ab0fca82bd7eb69d02b6b7a
a3fa4d6a6a QA: Fix bug in -usecli logic that converts booleans to non-lowercase strings (Jonas Schnelli)
4704e5f074 [QA] add createwallet disableprivatekey test (Jonas Schnelli)
c7b8f343e9 [Qt] Disable creating receive addresses when private keys are disabled (Jonas Schnelli)
2f15c2bc20 Add disable privatekeys option to createwallet (Jonas Schnelli)
cebefba085 Add option to disable private keys during internal wallet creation (Jonas Schnelli)
9995a602a6 Add facility to store wallet flags (64 bits) (Jonas Schnelli)
Pull request description:
This mode ('createwallet {"disableprivatekeys": true}') is intended for a sane pure watch-only mode, ideal for a use-case where one likes to use Bitcoin-Core in conjunction with a hardware-wallet or another solutions for cold-storage.
Since we have support for custom change addresses in `fundrawtransaction`, pure watch-only wallets including coin-selection are possible and do make sense for some use cases.
This new mode disables all forms of private key generation and ensure that no mix between hot and cold keys are possible.
Tree-SHA512: 3ebe7e8d54c4d4e5f790c348d4c292d456f573960a5b04d69ca5ef43a9217c7e7671761c6968cdc56f9a8bc235f3badd358576651af9f10855a0eb731f3fc508
Checks that all of the one byte type keys are actually one byte and
throw an error if they are not.
Add tests for each type to check for this behavior.
020628e3a4 Tests for PSBT (Andrew Chow)
a4b06fb42e Create wallet RPCs for PSBT (Andrew Chow)
c27fe419ef Create utility RPCs for PSBT (Andrew Chow)
8b5ef27937 SignPSBTInput wrapper function (Andrew Chow)
58a8e28918 Refactor transaction creation and transaction funding logic (Andrew Chow)
e9d86a43ad Methods for interacting with PSBT structs (Andrew Chow)
12bcc64f27 Add pubkeys and whether input was witness to SignatureData (Andrew Chow)
41c607f09b Implement PSBT Structures and un/serialization methods per BIP 174 (Andrew Chow)
Pull request description:
This Pull Request fully implements the [updated](https://github.com/bitcoin/bips/pull/694) BIP 174 specification. It is based upon #13425 which implements the majority of the signing logic.
BIP 174 specifies a binary transaction format which contains the information necessary for a signer to produce signatures for the transaction and holds the signatures for an input while the input does not have a complete set of signatures.
This PR contains structs for PSBT, serialization, and deserialzation code. Some changes to `SignatureData` have been made to support detection of UTXO type and storing public keys.
***
Many RPCs have been added to handle PSBTs.
`walletprocesspsbt` takes a PSBT format transaction, updates the PSBT with any inputs related to this wallet, signs, and finalizes the transaction. There is also an option to not sign and just update.
`walletcreatefundedpsbt` creates a PSBT from user provided data in the same form as createrawtransaction. It also funds the transaction and takes an options argument in the same form as `fundrawtransaction`. The resulting PSBT is blank with no input or output data filled in. It is analogous to a combination of `createrawtransaction` and `fundrawtransaction`
`decodepsbt` takes a PSBT and decodes it to JSON. It is analogous to `decoderawtransaction`
`combinepsbt` takes multiple PSBTs for the same tx and combines them. It is analogous to `combinerawtransaction`
`finalizepsbt` takes a PSBT and finalizes the inputs. If all inputs are final, it extracts the network serialized transaction and returns that instead of a PSBT unless instructed otherwise.
`createpsbt` is like `createrawtransaction` but for PSBTs instead of raw transactions.
`convertpsbt` takes a network serialized transaction and converts it into a psbt. The resulting psbt will lose all signature data and an explicit flag must be set to allow transactions with signature data to be converted.
***
This supersedes #12136
Tree-SHA512: 1ac7a79e5bc669933f0a6fcc93ded55263fdde9e8c144a30266b13ef9f62aacf43edd4cbca1ffbe003090b067e9643c9298c79be69d7c1b10231b32acafb6338
fabe28a0cd qa: Temporarily disable test that reads the default datadir location (MarcoFalke)
41a8c8dfaf travis: Check that ~/.bitcoin is never created (MarcoFalke)
Pull request description:
Tree-SHA512: d114db29a18f684d207caa0d7c947b13c945e2dd8b6d7fdeacdf7aa194f8123579d1139331b9d308df69a1132796e805a9ab63580aebde9b719860c0ff4b5652
Protects against added circular depencies, makes it explicit in the
code when circular dependencies have been removed.
Modeled after EXPECTED_BOOST_INCLUDES in lint-includes.sh
b9f4b211df tests: Use MAX_SCRIPT_ELEMENT_SIZE from script.py (Daniel Kraft)
Pull request description:
`p2p_segwit.py` and `test_framework/script.py` both define a constant for `MAX_SCRIPT_ELEMENT_SIZE` (=520 bytes), which is redundant. This change uses the constant defined in the `script.py` module for `p2p_segwit.py`.
Tree-SHA512: 2bc295ff26d9b052d4e05b85c27e748175884d6689a92c19337fc4db8bf439e3abe3edc91af1aaf46d8dc42ed96a85ad17110546d2274a0d9cda3abd6b878a31
be98b2d9a8 [QA] Add scantxoutset test (Jonas Schnelli)
eec7cf7b33 scantxoutset: mention that scanning by address will miss P2PK txouts (Jonas Schnelli)
94d73d32ab scantxoutset: support legacy P2PK script type (Jonas Schnelli)
892de1dfea scantxoutset: add support for scripts (Jonas Schnelli)
78304941f7 Blockchain/RPC: Add scantxoutset method to scan UTXO set (Jonas Schnelli)
9048575511 Add FindScriptPubKey() to search the UTXO set (Jonas Schnelli)
Pull request description:
Alternative to #9152.
This takes `<n>` pubkeys and optionally `<n>` xpubs (together with a definable lookup windows where the default is 0-1000) and looks up common scripts in the UTXO set of all given or derived keys.
The output will be an array similar to `listunspent`. That array is compatible with `createrawtransaction` as well as with `signrawtransaction`.
This makes it possible to prepare sweeps and have them signed in a secure (cold) space.
Tree-SHA512: a2b22a117cf6e27febeb97e5d6fe30184926d50c0c7cbc77bb4121f490fed65560c52f8eac67a9720d7bf8f420efa42459768685c7e7cc03722859f51a5e1e3b
Added functional tests for PSBT that test the RPCs. Also added all
of the BIP 174 test vectors (except for the updater tests) in the
functional tests.
Added a Unit test for the BIP 174 updater test vector.
89e70f9d7f Fix that CWallet::AbandonTransaction would only traverse one level (Ben Woosley)
Pull request description:
Prior to this change, it would mark only the first layer of
child transactions abandoned, due to always following the input `hashTx`
rather than the current `now` tx.
Tree-SHA512: df068b49637d299ad73237c7244005fe5aa966d6beae57aff12e6948f173d9381e1b5d08533f7e3a1416991ed57f9f1f7b834057141d85c07dc60bb1f0872cea
Commit 3fdb29778a renamed share/rpcuser to share/rpcauth but left references to the old path in code; this commit fixes the old references.
Performed update using https://github.com/facebook/codemod with command: `codemod --extensions cpp,py,md 'share/rpcuser' 'share/rpcauth'`
-BEGIN VERIFY SCRIPT-
git grep --files-with-matches 'share/rpcuser' src/*.cpp | xargs sed -i -E 's:share/rpcuser:share/rpcauth:g'
git grep --files-with-matches 'share/rpcuser' test/functional/*.py | xargs sed -i -E 's:share/rpcuser:share/rpcauth:g'
-END VERIFY SCRIPT-
backupwallet was broken for multiwallets in their own directories
(i.e. something like DATADIR/wallets/mywallet/wallet.dat). In this
case, the backup would use DATADIR/wallets/wallet.dat as source file
and not take the specific wallet's directory into account.
This led to either an error during the backup (if the wrong source
file was not present) or would silently back up the wrong wallet;
especially the latter behaviour can be quite bad for users.
38040c34e1 [tests] Remove accounts from wallet_importprunedfunds.py (John Newbery)
Pull request description:
This was split from #13075 to not block review/merge of that PR.
Tree-SHA512: 631d7139ed2bda5222ec395cc75720261e2e1f741dba04723d09fe04ef6cf92222a3679d886026ec33e2db2d1e2fa1a0f36c2451581d0f733a9939a98c7118ab
f40b3b82df [tests] functional test for createmultisig RPC (Anthony Towns)
b9024fdda3 segwit support for createmultisig RPC (Anthony Towns)
d58055d25f Move AddAndGetDestinationForScript from wallet to outputype module (Anthony Towns)
9a44db2e46 Add outputtype module (Anthony Towns)
Pull request description:
Adds an "address_type" parameter that accepts "legacy", "p2sh-segwit", and "bech32" to choose the type of address created. Defaults to "legacy" rather than the value of the `-address-type` option for backwards compatibility.
As part of implementing this, OutputType is moved from wallet into its own module, and `AddAndGetDestinationForScript` is changed to apply to a `CKeyStore` rather than a wallet, and to invoke `keystore.AddCScript(script)` itself rather than expecting the caller to have done that.
Fixes#12502
Tree-SHA512: a08c1cfa89976e4fd7d29caa90919ebd34a446354d17abb862e99f2ee60ed9bc19d8a21a18547c51dc3812cb9fbed86af0bef2f1e971f62bf95cade4a7d86237
702ae1e21a [RPC] [wallet] allow getbalance to use min_conf and watch_only without accounts. (John Newbery)
cf15761f6d [wallet] GetBalance can take a min_depth argument. (John Newbery)
0f3d6e9ab7 [wallet] factor out GetAvailableWatchOnlyBalance() (John Newbery)
7110c830f8 [wallet] deduplicate GetAvailableCredit logic (John Newbery)
ef7bc8893c [wallet] Factor out GetWatchOnlyBalance() (John Newbery)
4279da4785 [wallet] GetBalance can take an isminefilter filter. (John Newbery)
Pull request description:
#12953 inadvertently removed the functionality to call `getbalance "*" <int> <bool>` to get the wallet's balance with either minconfs or include_watchonly.
This restores that functionality (when `-deprecatedrpc=accounts`), and also makes it possible to call ``getbalance minconf=<int> include_watchonly=<bool>` when accounts are not being used.
Tree-SHA512: 67e84de9291ed6d34b23c626f4dc5988ba0ae6c99708d02b87dd3aaad3f4b6baa6202a66cc2dadd30dd993a39de8036ee920fcaa8cbb1c5dfe606e6fac183344
eeeef80fb6 qa: Fix some TODOs in p2p_segwit (MarcoFalke)
Pull request description:
* I believe we don't need to redundantly test versionbits logic in every functional tests that tests a softfork deployment that is being done with versionbits. Thus, remove two `TODO`s that ask for that.
* Replace another `TODO` with `wait_until`.
* Some style fixups after #13467
Tree-SHA512: c7120404d50579d6f3b9092f1e259959190eeafe520231e3479c8c256a50bf7260ccc93f8301ac0e100c54037053f6849433ebb1c55607e01d94b9812e525083
Prior to this change, it would mark only the first layer of
child transactions abandoned, due to always following the input hashTx
rather than the current now tx.
p2p_segwit.py and test_framework/script.py both define a constant for
MAX_SCRIPT_ELEMENT_SIZE (=520 bytes), which is redundant. This change
uses the constant defined in the script.py module for p2p_segwit.py.
685d1d8115 [tests] Check signrawtransaction* errors on missing prevtx info (Anthony Towns)
a3b065b51f Error on missing amount in signrawtransaction* (Anthony Towns)
Pull request description:
Signatures using segregated witness commit to the amount being spent, so that value must be passed into signrawtransactionwithkey and signrawtransactionwithwallet. This ensures an error is issued if that doesn't happen, rather than just assuming the value is 0 and producing a signature that is almost certainly invalid.
Based on Ben Woosley's #12458, Fixes: #12429.
Tree-SHA512: 8e2ff89d5bcf79548e569210af0d850028bc98d86c149b92207c9300ab1d63664a7e2b222c1be403a15941aa5cf36ccc3c0d570ee1c1466f3496b4fe06c17e11
d280617bf5 [qa] Add a test for merkle proof malleation (Suhas Daftuar)
ed82f17000 have verifytxoutproof check the number of txns in proof structure (Gregory Sanders)
Pull request description:
Recent publication of a weakness in Bitcoin's merkle tree construction demonstrates many SPV applications vulnerable to an expensive to pull off yet still plausible attack: https://bitslog.wordpress.com/2018/06/09/leaf-node-weakness-in-bitcoin-merkle-tree-design/
This change would at least allow `verifytxoutproof` to properly validate that the proof matches a known block, with known number of transactions any time after the full block is processed. This should neuter the attack entirely.
The negative is that a header-only processed block/future syncing mode would cause this to fail until the node has imported the data required.
related: #13451
`importprunedfunds` needs this check as well. Can expand it to cover this if people like the idea.
Tree-SHA512: 0682ec2b622a38b29f3f635323e0a8b6fc071e8a6fd134c954579926ee7b516e642966bafa667016744ce49c16e19b24dbc8801f982a36ad0a6a4aff6d93f82b
57889e688d bitcoin-tx: Stricter check for valid integers (Daniel Kraft)
Pull request description:
Just calling `atoi` to convert strings to integers does not check for valid integers very thoroughly; in particular, it just ignores everything starting from the first non-numeral character. Even a string like "foo" is fine and silently returns 0.
This meant that `bitcoin-tx` would not fail if such a string was passed in various places where an integer is expected (like the `locktime` or an input/output index); this means that it would, for instance, silently accept a typo and interpret it in an unexpected way.
In this change, we use `ParseInt64` for parsing strings to integers, which actually verifies that the full string is valid as number. New tests in the `bitcoin-util-test` cover the new error paths.
This fixes#13599.
Tree-SHA512: 146a0af275e9f57784e5d0582d3defbac35551b54b6b7232f8a0b20db04aa611125e52aa4512ef2f8ed2cafc2a12fe586f9d10ed66d641cff090288f279b1988
161e8d40a4 RPC: Add new getzmqnotifications method. (Daniel Kraft)
caac39b0ac Make ZMQ notification interface instance global. (Daniel Kraft)
Pull request description:
This adds a new RPC method `getzmqnotifications`, which returns information about all active ZMQ notification endpoints. This is useful for software that layers on top of bitcoind, so it can verify that ZeroMQ is enabled and also figure out where it should listen.
See #13526.
Tree-SHA512: edce722925741c84ddbf7b3a879fc9db1907e5269d0d97138fe724035d93ee541c2118c24fa92f4197403f380d0e25c2fda5ca6c62d526792ea749cf527a99a0
ea65182f03 [wallet] loadwallet shouldn't create new wallets. (John Newbery)
Pull request description:
A bug in the initial implementation of loadwallet meant that if the
arguement was a directory that didn't contain a wallet.dat file, a new
wallet would be created in that directory. Fix that so that if a
directory is passed in, it must contain a wallet.dat file.
Bug reported by promag (João Barbosa).
Tree-SHA512: 0a59fa8a33fde51a88544ad288b00e4995284fe16424f643076aaba42b8244fff362145217650ee53d518dfab7efbed4237632c34cdd3dcbbecaa9ecaab5fd7b
Just calling atoi to convert strings to integers does not check for
valid integers very thoroughly; in particular, it just ignores
everything starting from the first non-numeral character. Even a string
like "foo" is fine and silently returns 0.
This meant that bitcoin-tx would not fail if such a string was passed in
various places where an integer is expected (like the locktime or an
input/output index); this means that it would, for instance, silently
accept a typo and interpret it in an unexpected way.
In this change, we use ParseInt64 for parsing strings to integers,
which actually verifies that the full string is valid as number.
New tests in the bitcoin-util-test cover the new error paths.
e3aab295e [tests] p2p_segwit: sync_blocks in subtest wrapper. (John Newbery)
55e805085 [tests] p2p_segwit: remove unnecessary arguments from subtests. (John Newbery)
25711c269 [tests] p2p_segwit: log and assert segwit status in subtest wrapper. (John Newbery)
6839863d5 [tests] p2p_segwit: Make sure each subtest leaves utxos for the next. (John Newbery)
bfe32734d [tests] p2p_segwit: wrap subtests with subtest wrapper. (John Newbery)
2af4e398d [tests] p2p_segwit: re-order function definitions. (John Newbery)
94a0134a4 [tests] p2p_segwit: standardise comments/docstrings. (John Newbery)
f7c7f8ecf [tests] p2p_segwit: Fix flake8 warnings. (John Newbery)
Pull request description:
`p2p_segwit.py` is a very long test, composed of multiple subtests. When it fails it's difficult to debug for a couple of reasons:
- Control flow jumps between different methods in the test class, so it's a little difficult to follow the code.
- state may be carried forward unintentionally from one subtest to the next.
Improve that by wrapping the subtests with a `@subtest` decorator which:
- logs progress
- asserts state after each subtest
As usual, I've also included a few commits which generally tidy up the test and improve style.
Tree-SHA512: 3650602b3ce9823dc968cc5f2e716757feadc3dbedb3605eb79bb3df91a6db8ae53431f253b440da690e3a8e9d76de84fad4368a2663aeb40e6b9427cf948870
This adds a new RPC method "getzmqnotifications", which returns
information about all active ZMQ notification endpoints. This is useful
for software that layers on top of bitcoind, so it can verify that
ZeroMQ is enabled and also figure out where it should listen.
See https://github.com/bitcoin/bitcoin/issues/13526.
75848bcf40 [tests] Fix p2p_sendheaders race (John Newbery)
Pull request description:
p2p_sendheaders has a race in part 1.3.
part 1.2 sends a block to the node over the 'test_node' connection, but
doesn't wait for an inv to be received on the 'inv_node' connection. If
we get to part 1.3 before that inv has been received, then the
subsequent call to check_last_inv_announcement could fail.
Tree-SHA512: ba9baffb3a9c0d379259190c737a7a4ad2e1133005a5b026af4f6b67a2978e24db39289551ad29134151879593ef5472be7e569a3557c0740fb51f5c56263d9a
fa87da2f17 qa: Avoid start/stop of the network thread mid-test (MarcoFalke)
Pull request description:
This simplifies test writing by removing the need to handle the network thread in tests. E.g. start thread, join thread, restart thread mid-test, adding p2p connections at the "right" time, ...
Tree-SHA512: 533642f12fef5496f1933855edcdab1a7ed901d088d34911749cd0f9e044c8a6cb1f89985ac3a7f41a512943663e4e270a61978f6f072143ae050cd102d4eab8
A bug in the initial implementation of loadwallet meant that if the
arguement was a directory that didn't contain a wallet.dat file, a new
wallet would be created in that directory. Fix that so that if a
directory is passed in, it must contain a wallet.dat file.
Bug reported by promag (João Barbosa).
fa103a5d5e [qa] wallet_basic: Specify minimum required amount for listunspent (MarcoFalke)
Pull request description:
A value less than that would fail the tests later on anyway:
```
File "./test/functional/wallet_basic.py", line 250, in run_test
self.nodes[1].sendrawtransaction(signed_raw_tx['hex'])
test_framework.authproxy.JSONRPCException: bad-txns-in-belowout, value in (1.00) < value out (49.998) (code 16) (-26)
Tree-SHA512: 7e72ad02b5623bc078610da06c34721836822a920a4e85b12a1e0f339e3205cdc11d39763197770e649fb73376f922ff91a8f244b465195e50a6798658e04f80
df10f07db1 [wallet] Don't use accounts when checking balance in sendmany (John Newbery)
e209184101 [wallet] deprecate sendfrom RPC method. (John Newbery)
Pull request description:
A couple of fixups from the accounts API deprecation PR (#12953):
- properly deprecate `sendfrom`
- don't use accounts when calculating balance in `sendmany` (unless the `-deprecatedrpc=accounts` flag is being used)
Tree-SHA512: 1befde055067438c4c3391bbff1aaed0e6249efd708c567db3f1faad40a0f28e64f95e5bad0679ae826d24a0239e4bc8a1c392dc93e2e7502343a7f6b1d1845c
927e1150bc Test: Harden lint-filenames.sh (wodry)
Pull request description:
- This fixes that only files with lower case file name suffix where found before, which contradicted the Regex to find uppercase characters in file names (including suffixes I guess).
- `--full-name` switch was added to git ls-files, to define that the found file always includes it's full path in the git project.
- since we know now that the file name includes the full path, we can harden the Regex to exclude the secp256k1 and univalue sub folders.
- use backslash line break to make code easier to read and avoid too long line.
Tree-SHA512: 9b55fe4965ae2084112b9f8a81bf9c657756c2cb5004986e7b6102a76adaf62c7d7a53257d9f13c5d8a1c75870b52c744d13830e3edd454a099e810357c914e5
000000035b Obsolete #!/bin/bash shebang (DesWurstes)
Pull request description:
> `#!/bin/bash` assumes it is always installed to `/bin/` which can cause issues
> `#!/usr/bin/env bash` searches the user's `PATH` to find the `bash` binary
Details: https://github.com/dylanaraps/pure-bash-bible#obsolete-syntax
I'm open to comments: Should I also fix `#!/bin/sh`?
Tree-SHA512: b47bb4828116aa119f1899c68fee081270d51a898535490b9c616bf0f3660ad953f29c361eafc759bc64cdd54ee6eeecb2d79e9fdb5291a996a515c719805476
fa1eac9cdb [qa] mininode: Expose connection state through is_connected (MarcoFalke)
Pull request description:
This gets rid of some non-type safe string comparisons and access to members that are implementation details of `class P2PConnection(asyncore.dispatcher)`. Such refactoring is required to replace the deprecated asyncore with something more sane.
Changes:
* Get rid of non-enum member `state` and replace is with bool `connected`
* Get rid of confusing argument `pushbuf` and literally just push to the buffer at the call site
Tree-SHA512: 09074c7e5ed251a2e0509ef205ab82f89887c1e1fa1cc6efc1db60d196eb2403788a4987df8809fd06d80ef652e614c5d3c3fdef70096fc5815102243388288d
p2p_sendheaders has a race in part 1.3.
part 1.2 sends a block to the node over the 'test_node' connection, but
doesn't wait for an inv to be received on the 'inv_node' connection. If
we get to part 1.3 before that inv has been received, then the
subsequent call to check_last_inv_announcement could fail.
47776a958b Add linter: Make sure all shell scripts opt out of locale dependence using "export LC_ALL=C" (practicalswift)
3352da8da1 Add "export LC_ALL=C" to all shell scripts (practicalswift)
Pull request description:
~~Make sure `LC_ALL=C` is set when using `grep` range expressions.~~
Make sure `LC_ALL=C` is set in all shell scripts.
From the `grep(1)` documentation:
> Within a bracket expression, a range expression consists of two characters separated by a hyphen. It matches any single character that sorts between the two characters, inclusive, using the locale's collating sequence and character set. For example, in the default C locale, `[a-d]` is equivalent to `[abcd]`. Many locales sort characters in dictionary order, and in these locales `[a-d]` is typically not equivalent to `[abcd]`; it might be equivalent to `[aBbCcDd]`, for example. To obtain the traditional interpretation of bracket expressions, you can use the C locale by setting the `LC_ALL` environment variable to the value C.
Context: [Locale issue found when reviewing #13450](https://github.com/bitcoin/bitcoin/pull/13450/files#r194877736)
Tree-SHA512: fd74d2612998f9b49ef9be24410e505d8c842716f84d085157fc7f9799d40e8a7b4969de783afcf99b7fae4f91bbb4559651f7dd6578a6a081a50bdea29f0909
c8176b3cc7 Add linter: Make sure we explicitly open all text files using UTF-8 or ASCII encoding in Python (practicalswift)
634bd97001 Explicitly specify encoding when opening text files in Python code (practicalswift)
Pull request description:
Add linter: Make sure we explicitly open all text files using UTF-8 encoding in Python.
As requested by @laanwj in #13440.
Tree-SHA512: 1651c00fe220ceb273324abd6703aee504029b96c7ef0e3029145901762c733c9b9d24927da281394fd4681a5bff774336c04eed01fafea997bb32192c334c06
86edf4a2a5 expose CBlockIndex::nTx in getblock(header) (Gregory Sanders)
Pull request description:
Recent publication of a weakness in Bitcoin's merkle tree construction demonstrates many SPV applications vulnerable to an expensive to pull off yet still plausible attack: https://bitslog.wordpress.com/2018/06/09/leaf-node-weakness-in-bitcoin-merkle-tree-design/
Including the coinbase in the txoutproof seems the most effective fix, however results in a significant efficiency downgrade. Transactors will not even know a priori what the size of their proof will be within a couple orders of magnitude, unless they use the mid-state of SHA2 as detailed in the blog post.
Some applications, like Elements blockchain platform that take SPV-style proofs have optional access to a bitcoind to verify these proofs of inclusion and check depth in the chain. Returning `CBlockIndex::nTx` would allow an extremely easy and compact way of checking the depth of the tree, with no additional overhead to the codebase, and works with pruned nodes.
`getblockheader` is arguably not the place for it, but as mentioned before, is a natural workflow for us checking depth of a block in a possibly pruned node.
We should also ensure that `verifytxoutproof` ends up validating this depth fact as well, but left this for another PR.
Tree-SHA512: af4cf48e704c6088f8da06a477fda1aaa6f8770cee9b876c4465d1075966d6a95831a88817673fe5a0d6bbcdc1ffcbc1892e2be0d838c60fc6958d33eacdcc14
fa7a6cf1b3 policy: Treat segwit as always active (MarcoFalke)
Pull request description:
Now that segwit is active for a long time, there is no need to reject transactions with the reason that segwit hasn't activated.
Strictly speaking, this is a bug fix, because with the release of 0.16, we create segwit transactions in our wallet by default without checking if they are allowed by local policy.
More broadly, this simplifies the code as if "premature witness" was always set to true with the corresponding command line args.
Tree-SHA512: 484c26aa3a66faba6b41e8554a91a29bfc15fbf6caae3d5363a3966283143189c4bd5333a610b0669c1238f75620691264e73f6b9f1161cdacf7574d946436da
fa8071a098 qa: Log as utf-8 (MarcoFalke)
Pull request description:
Explicitly read and write the log files with utf-8 as encoding
Tree-SHA512: ca28f37f34a09845c736ff6c4c21733c3c39584f52c81e48ff25e5e35979c317d0989862b2b93acc7e359fbcc20b99533365455830b2ddb41eb4d8c17314534e
16e3cd380a Clarify include recommendation (practicalswift)
6d10f43738 Enforce the use of bracket syntax includes ("#include <foo.h>") (practicalswift)
906bee8e5f Use bracket syntax includes ("#include <foo.h>") (practicalswift)
Pull request description:
When analysing includes in the project it is often assumed that the preferred bracket include syntax (`#include <foo.h>`) mentioned in `developer-docs.md` is used consistently. @sipa:s excellent circular dependencies script [`circular-dependencies.py`](50c69b7801/contrib/devtools/circular-dependencies.py) (#13228) is an example of a script making this reasonable assumption.
This PR enables automatic Travis checking of the include syntax making sure that the bracket syntax includes (`#include <foo.h>`) is used consistently.
Tree-SHA512: a414921aabe8e487ebed42f3f1cbd02fecd1add385065c1f2244cd602c31889e61fea5a801507ec501ef9bd309b05d3c999f915cec1c2b44f085bb0d2835c182
67e0e04140 [wallet] [docs] Update release notes for removing `getlabeladdress` (John Newbery)
81608178cf [wallet] [rpc] Remove getlabeladdress RPC (John Newbery)
Pull request description:
labels are associated with addresses (rather than addresses being
associated with labels, as was the case with accounts). The
getlabeladdress does not make sense in this model, so remove it.
getaccountaddress is still supported for one release as the accounts
API is deprecated.
Tree-SHA512: 7f45d0456248ebcc4e54dd34e2578a09a8ea8e4fceda75238ccea9d731dc99a3f3c0519b18a9739de17d2e6e59c9c2259ba67c9ae2e3cb2a40ddb14b9193fe29
698cfd0811 docs: Mention lint-locale-dependence.sh in developer-notes.md (practicalswift)
0a4ea2f458 build: Add linter for checking accidental locale dependence (practicalswift)
Pull request description:
This linter will check for code accidentally introducing locale dependencies.
Unnecessary locale dependence can cause bugs that are very tricky to isolate and fix. We should avoid using locale dependent functions if possible.
Context: https://github.com/bitcoin/bitcoin/pull/12881#issuecomment-378564722
Example output:
```
$ contrib/devtools/lint-locale-dependence.sh
The locale dependent function tolower(...) appears to be used:
src/init.cpp: if (s[0] == '0' && std::tolower(s[1]) == 'x') {
Unnecessary locale dependence can cause bugs that are very
tricky to isolate and fix. Please avoid using locale dependent
functions if possible.
Advice not applicable in this specific case? Add an exception
by updating the ignore list in contrib/devtools/lint-locale-dependence.sh
```
**Note to reviewers:** What is the most appropriate `LOCALE_DEPENDENT_FUNCTIONS` function list? What should be added or removed?
Tree-SHA512: 14e448828804bb02bf59070647e38b52fce120c700c903a4a8472769a2cee5dd529bd3fc182386993cb8720482cf4250b63a0a477db61b941ae4babe5c65025f
fa4760fbb3 qa: Increase includeconf test coverage (MarcoFalke)
Pull request description:
This adds some missing `return false` for error conditions and adds test coverage [1] for those.
Also, extend recursion warning when the chain was set in one of the includeconfs.
[1] See the red lines in https://marcofalke.github.io/btc_cov/total.coverage/src/util.cpp.gcov.html for missing coverage.
Tree-SHA512: d32563c9bb277879895a173e699034db5ecdb4061a1ec8890c566d61e36a09efa5eda19a029baf952ff6d568f8b9684a13a0bb90827850075470975e2088fee4
faac7a2db4 qa: Avoid checking reject code for now (MarcoFalke)
Pull request description:
The node will often disconnect before sending a reject code. A more
robust solution would be to read from the debug log. See #13006
Tree-SHA512: 1dabf8a43dabbc722f4ffe4fbc1f870090253a66290b2d1a95e7a24e14c6442b493c314480c0314587164eb65e5d468aa9eb5e107ad90bb3ca821a97ea4d373c
fa26cf0156 qa: Fixup setting of PATH env var (MarcoFalke)
Pull request description:
This was an oversight of mine in #13188
Can be trivially tested with `BITCOIND=bitcoin-qt ./test/functional/wallet_disable.py` before and after this fix.
Tree-SHA512: 06c7b2f12158855eb2b6392861943821bd7ad3152cf0dd49ac4abd878e5b937ebee55e256ce5bdc1c2a9c775a452112c34533366c934ff5f0f412b3a7e1c8118
fa3c910bfe test: Move linters to test/lint, add readme (MarcoFalke)
Pull request description:
This moves the checks and linters from `devtools` to a subfolder in `test`. (Motivated by my opinion that the dev tools are mostly for generating code and updating the repo whereas the linters are read-only checks.)
Also, adds a readme to clarify that checks and linters are only meant to prevent bugs and user facing issues, not merely stylistic preference or inconsistencies. (This is motivated by the diversity in developers and work flows as well as existing code styles. It would be too disruptive to change all existing code to a single style or too burdensome to force all developers to adhere to a single style. Also note that our style guide is changing, so locking in at the wrong style "too early" would only waste resources.)
Tree-SHA512: 9b10e89f2aeaf0c8a9ae248aa891d74e0abf0569f8e5dfd266446efa8bfaf19f0ea0980abf0b0b22f0d8416ee90d7435d21a9f9285b66df43f370b7979173406
87fe292d89 doc: Mention disabling BIP61 in bips.md (Wladimir J. van der Laan)
fe16dd8226 net: Add option `-enablebip61` to configure sending of BIP61 notifications (Wladimir J. van der Laan)
Pull request description:
This commit adds a boolean option `-peersendreject`, defaulting to `1`, that can be used to disable the sending of [BIP61](https://github.com/bitcoin/bips/blob/master/bip-0061.mediawiki) `reject` messages. This functionality has been requested for various reasons:
- security (DoS): reject messages can reveal internal state that can be used to target certain resources such as the mempool more easily.
- bandwidth: a typical node sends lots of reject messages; this counts against upstream bandwidth. Also the reject messages tend to be larger than the message that was rejected.
On the other hand, reject messages can be useful while developing client software (I found them indispensable while creating bitcoin-submittx), as well as for our own test cases, so whatever the default becomes on the long run, IMO the functionality should be retained as option. But that's a discussion for later, for now it's simply a node operator decision.
Also adds a RPC test that checks the functionality.
Tree-SHA512: 9488cc53e13cd8e5c6f8eb472a44309572673405c1d1438c3488f627fae622c95e2198bde5ed7d29e56b948e2918bf1920239e9f865889f4c37c097c37a4d7a9
