Commit Graph

2023 Commits

Author SHA1 Message Date
James Chiang
2493770e36 TestShell: Return self from setup()
This allows user to chain setup() to the initializer. test-shell.md code
examples have been updated to reflect this.
2019-11-05 12:55:52 +01:00
James Chiang
a8dea45524 TestShell: Simplify default setting of num_nodes 2019-11-05 12:55:52 +01:00
James Chiang
9c7806e4bf Doc: Remove backticks in test-shell.md code block 2019-11-05 12:55:52 +01:00
James Chiang
d3ed06e2cd TestShell: Fix typo in TestShell warning printout 2019-11-05 12:55:52 +01:00
Samuel Dobson
bdda137878
Merge #16766: wallet: Make IsTrusted scan parents recursively
4671fc3d9e Expand on wallet_balance.py comment from https://github.com/bitcoin/bitcoin/pull/16766\#issuecomment-527563982 (Jeremy Rubin)
91f3073f08 Update release notes to mention changes to IsTrusted and impact on wallet (Jeremy Rubin)
8f174ef112 Systematize style of IsTrusted single line if (Jeremy Rubin)
b49dcbedf7 update variable naming conventions for IsTrusted (Jeremy Rubin)
5ffe0d1449 Update comment in test/functional/wallet_balance.py (Jeremy Rubin)
a550c58267 Update wallet_balance.py test to reflect new behavior (Jeremy Rubin)
5dd7da4ccd Reuse trustedParents in looped calls to IsTrusted (Jeremy Rubin)
595f09d6de Cache tx Trust per-call to avoid DoS (Jeremy Rubin)
dce032ce29 Make IsTrusted scan parents recursively (Jeremy Rubin)

Pull request description:

  This slightly modifies the behavior of IsTrusted to recursively check the parents of a transaction. Otherwise, it's possible that a parent is not IsTrusted but a child is. If a parent is not trusted, then a child should not be either.

  This recursive scan can be a little expensive, so ~it might be beneficial to have a way of caching IsTrusted state, but this is a little complex because various conditions can change between calls to IsTrusted (e.g., re-org).~ I added a cache which works per call/across calls, but does not store the results semi-permanently. Which reduces DoS risk of this change. There is no risk of untrusted parents causing a resource exploitation, as we immediately return once that is detected.

  This is a change that came up as a bug-fix esque change while working on OP_SECURETHEBAG. You can see the branch where this change is important here: https://github.com/bitcoin/bitcoin/compare/master...JeremyRubin:stb-with-rpc?expand=1. Essentially, without this change, we can be tricked into accepting an OP_SECURETHEBAG output because we don't properly check the parents. As this was a change which, on its own, was not dependent on OP_SECURETHEBAG, I broke it out as I felt the change stands on its own by fixing a long standing wallet bug.

  The test wallet_balance.py has been corrected to meet the new behavior. The below comment, reproduced, explains what the issue is and the edge cases that can arise before this change.

          # Before `test_balance()`, we have had two nodes with a balance of 50
          # each and then we:
          #
          # 1) Sent 40 from node A to node B with fee 0.01
          # 2) Sent 60 from node B to node A with fee 0.01
          #
          # Then we check the balances:
          #
          # 1) As is
          # 2) With transaction 2 from above with 2x the fee
          #
          # Prior to #16766, in this situation, the node would immediately report
          # a balance of 30 on node B as unconfirmed and trusted.
          #
          # After #16766, we show that balance as unconfirmed.
          #
          # The balance is indeed "trusted" and "confirmed" insofar as removing
          # the mempool transactions would return at least that much money. But
          # the algorithm after #16766 marks it as unconfirmed because the 'taint'
          # tracking of transaction trust for summing balances doesn't consider
          # which inputs belong to a user. In this case, the change output in
          # question could be "destroyed" by replace the 1st transaction above.
          #
          # The post #16766 behavior is correct; we shouldn't be treating those
          # funds as confirmed. If you want to rely on that specific UTXO existing
          # which has given you that balance, you cannot, as a third party
          # spending the other input would destroy that unconfirmed.
          #
          # For example, if the test transactions were:
          #
          # 1) Sent 40 from node A to node B with fee 0.01
          # 2) Sent 10 from node B to node A with fee 0.01
          #
          # Then our node would report a confirmed balance of 40 + 50 - 10 = 80
          # BTC, which is more than would be available if transaction 1 were
          # replaced.

  The release notes have been updated to note the new behavior.

ACKs for top commit:
  ariard:
    Code Review ACK 4671fc3, maybe extend DoS protection in a follow-up PR.
  fjahr:
    Code review ACK 4671fc3d9e
  ryanofsky:
    Code review ACK 4671fc3d9e. Changes since last review: 2 new commits adding suggested release note and python test comment, also a clean rebase with no changes to the earlier commits. The PR description is more comprehensive now, too. Looks good!
  promag:
    Code review ACK 4671fc3d9e.

Tree-SHA512: 6b183ff425304fef49724290053514cb2770f4a2350dcb83660ef24af5c54f7c4c2c345b0f62bba60eb2d2f70625ee61a7fab76a7f491bb5a84be5c4cc86b92f
2019-11-05 21:59:27 +13:00
Samuel Dobson
bfc4c896d6
Merge #17258: Fix issue with conflicted mempool tx in listsinceblock
436ad43643 Fix issue with conflicted mempool tx in listsinceblock (Adam Jonas)

Pull request description:

  Closes #8752 by bringing back abandoned #10470.

  This now checks that returned transactions are not conflicting with any transactions that are filtered out by the given blockhash and add a functional test to prevent this in the future.

  For more context, #8757 was closed in favor of #10470.

ACKs for top commit:
  instagibbs:
    utACK 436ad43643
  kallewoof:
    utACK 436ad43643
  jonatack:
    I'm not qualifed to give an ACK here but 436ad43643 appears reasonable. Built/ran tests/verified that this test fails without the change in rpcwallet.cpp:

Tree-SHA512: 63d75cd3d3f19fc84dc38899b200c96179b82b24db263cd0116ee5b715265be647157855c2e35912d2fbc49c7b37db9375d6aab0ac672f0f09bece8431de5ea9
2019-11-05 21:56:34 +13:00
MarcoFalke
bc38bb9a60
Merge #17288: Added TestShell class for interactive Python environments.
19139ee034 Add documentation for test_shell submodule (JamesC)
f5112369cf Add TestShell class (James Chiang)
5155602a63 Move argparse() to init() (JamesC)
2ab01462f4 Move assert num_nodes is set into main() (JamesC)
614c645643 Clear TestNode objects after shutdown (JamesC)
6f40820757 Add closing and flushing of logging handlers (JamesC)
6b71241291 Refactor TestFramework main() into setup/shutdown (JamesC)
ede8b7608e Remove network_event_loop instance in close() (JamesC)

Pull request description:

  This PR refactors BitcoinTestFramework to encapsulate setup and shutdown logic into dedicated methods, and adds a  ~~TestWrapper~~ TestShell child class. This wrapper allows the underlying BitcoinTestFramework to run _between user inputs_ in a REPL environment, such as a Jupyter notebook or any interactive Python3 interpreter.

  The ~~TestWrapper~~ TestShell is motivated by the opportunity to expose the test-framework as a prototyping and educational toolkit. Examples of code prototypes enabled by  ~~TestWrapper~~ TestShell can be found in the Optech [Taproot/Schnorr](https://github.com/bitcoinops/taproot-workshop) workshop repository.

  Usage example:
  ```
  >>> import sys
  >>> sys.path.insert(0, "/path/to/bitcoin/test/functional")
  ```
  ```
  >>> from test_framework.test_wrapper import TestShell
  >>> test = TestShell()
  >>> test.setup(num_nodes=2)
  20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework (INFO): Initializing test directory /path/to/bitcoin_func_test_XXXXXXX
  ```
  ```
  >>> test.nodes[0].generate(101)
  >>> test.nodes[0].getblockchaininfo()["blocks"]
  101
  ```
  ```
  >>> test.shutdown()
  20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework (INFO): Stopping nodes
  20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework (INFO): Cleaning up /path/to/bitcoin_func_test_XXXXXXX on exit
  20XX-XX-XXTXX:XX:XX.XXXXXXX TestFramework (INFO): Tests successful
  ```

  **Overview of changes to BitcoinTestFramework:**

  - Code moved to `setup()/shutdown()` methods.
  - Argument parsing logic encapsulated by `parse_args` method.
  - Success state moved to `BitcoinTestFramework.success`.

  _During Shutdown_

  - `BitcoinTestFramework` logging handlers are flushed and removed.
  - `BitcoinTestFrameowork.nodes` list is cleared.
  - `NetworkThread.network_event_loop` is reset. (NetworkThread class).

  **Behavioural changes:**
  - Test parameters can now also be set when overriding BitcoinTestFramework.setup() in addition to overriding `set_test_params` method.
  - Potential exceptions raised in BitcoinTestFramework.setup() will be handled in main().

  **Added files:**
  - ~~test_wrapper.py~~ `test_shell.py`
  - ~~test-wrapper.md~~ `test-shell.md`

ACKs for top commit:
  jamesob:
    ACK 19139ee034
  jonatack:
    ACK 19139ee034
  jnewbery:
    Rather than invalidate the three ACKs for a minor nit, can you force push back to 19139ee034 please? I think this PR was ready to merge before your last force push.
  jachiang:
    > Rather than invalidate the three ACKs for a minor nit, can you force push back to [19139ee](19139ee034) please? I think this PR was ready to merge before your last force push.
  jnewbery:
    ACK 19139ee034

Tree-SHA512: 0c24f405f295a8580a9c8f1b9e0182b5d753eb08cc331424616dd50a062fb773d3719db4d08943365b1f42ccb965cc363b4bcc5beae27ac90b3460b349ed46b2
2019-11-04 14:54:14 -05:00
MarcoFalke
94a26b192f
Merge #17318: replace asserts in RPC code with CHECK_NONFATAL and add linter
c98bd13e67 replace asserts in RPC code with CHECK_NONFATAL and add linter (Adam Jonas)

Pull request description:

  - Replace instances of assert in /rpc files and rpcwallet with CHECK_NONFATAL(condition)
  - Add a linter to prevent future usage of assert being used in RPC code

  ref https://github.com/bitcoin/bitcoin/pull/17192

ACKs for top commit:
  practicalswift:
    ACK c98bd13e67 -- diff looks correct

Tree-SHA512: a16036b6bbcca73a5334665f66e17e1756377d582317568291da1d727fc9cf8c84bac9d9bd099534e1be315345336e5f7b66b93793135155f320dc5862a2d875
2019-11-04 11:33:41 -05:00
MarcoFalke
6cb10c14c6
Merge #17199: test: use default address type (bech32) for wallet_bumpfee tests
8d8e5a79d0 test: use default address type (bech32) for wallet_bumpfee tests (Sebastian Falbesoner)

Pull request description:

  The use of native segwit addresses (pure p2wpkh instead of p2sh-p2wpkh) leads to smaller transaction sizes, needing adaption of some constants in the following test cases:
  - `test_dust_to_fee()`: adaption of dust calculation (p2wpkh spend estimate of 67 is taken from `src/policy/policy.cpp:GetDustThreshold()`)
  - `test_maxtxfee_fails()`: lowering `-maxtxfee` setting to trigger fail

Top commit has no ACKs.

Tree-SHA512: b4163700d56c11955f811bc5fe6edaf7aec69931d7db741c03b055fb518bb9825c031fb931c513b37a1968085cb8c2f263adf664b357aff8ee42795fd0f88d2d
2019-11-04 11:27:48 -05:00
JamesC
19139ee034 Add documentation for test_shell submodule 2019-11-04 16:02:28 +01:00
randymcmillan
ac831339cb
doc: Fix some misspellings 2019-11-04 04:22:53 -05:00
James Chiang
f5112369cf Add TestShell class
A BitcoinTestFramework child class which can be imported by an external user or
project. TestShell.setup() initiates an underlying BitcoinTestFramework object
with bitcoind subprocesses, rpc interfaces and test logging.
TestShell.shutdown() safely tears down the BitcoinTestFramework object.
2019-11-04 08:56:56 +01:00
JamesC
5155602a63 Move argparse() to init()
This ensures TestFramework default parameters are set before setup is called. A
 child class will therefore have access to defaults when overriding setup.
2019-11-03 20:34:49 +01:00
JamesC
2ab01462f4 Move assert num_nodes is set into main()
This allows a BitcoinTestFramework child class to set test parameters in an
overridden setup() rather than in an overridden set_test_params().
2019-11-03 20:34:41 +01:00
JamesC
614c645643 Clear TestNode objects after shutdown
TestNode objects need to be removed during shutdown, as setup_nodes does not
remove previous TestNode objects from previous test runs during setup.
2019-11-03 20:34:27 +01:00
JamesC
6f40820757 Add closing and flushing of logging handlers
In order for BitcoinTestFramework to correctly restart after shutdown, the
previous logging handlers need to be removed, or else logging will continue in
the previous temp directory. "Flush" ensures buffers are emptied, and "close"
ensures file handler close logging file.
2019-11-03 20:34:18 +01:00
JamesC
6b71241291 Refactor TestFramework main() into setup/shutdown
Setup and shutdown code now moved into dedicated methods. Test "success" is
added as a BitcoinTestFramework member, which can be accessed outside of main.
Argument parsing also moved into separate method and called from main.
2019-11-03 20:34:07 +01:00
JamesC
ede8b7608e Remove network_event_loop instance in close()
The asyncio.new_event_loop() instance is now removed from the NetworkThread
class during shutdown. This enables a NetworkThread instance to be restarted
after being closed. The current NetworkThread class guards against an existing
new_event_loop during initialization.
2019-11-03 20:33:49 +01:00
MarcoFalke
6a7c40bee4
Merge #15888: QA: Add wallet_implicitsegwit to test the ability to transform keys between address types
a6f6f77a86 QA: Add wallet_implicitsegwit to test the ability to transform keys between address types (Luke Dashjr)

Pull request description:

  This makes sure the wallet recognises payments to keys via address types they weren't created with.

  While we don't *want* this behaviour, it might make sense to explicitly test that it works until we remove it.

ACKs for top commit:
  adamjonas:
    utACK a6f6f77a86

Tree-SHA512: b208405729277e9ce06eb772b45e8d1683c4dc5703754448b8f19a590b37522abd7bb46d4dbd41513b3d46d7f9e8769ce4f15fa4114be600f31a1ebbc1157840
2019-11-01 13:24:50 -04:00
MarcoFalke
bc3fcf3c0d
Merge #17327: test: add rpc_fundrawtransaction logging
ff22751417 test: rm ascii art in rpc_fundrawtransaction (Jon Atack)
94fcc08541 test: add rpc_fundrawtransaction logging (Jon Atack)

Pull request description:

  `test/functional/rpc_fundrawtransaction.py` is fairly slow to run and has no logging, so it can appear to be stalled.

  This commit adds info logging at each test to provide feedback on the test run.

ACKs for top commit:
  instagibbs:
    utACK ff22751417
  jnewbery:
    tACK ff22751417

Tree-SHA512: f4fabad8ef51c29981351bb4e66fb0c0e0517418a4a15892ef804df11d16b2d2ae1a1abc958d2b121819850278de90a2003b0edb8d7098d00360b89fa76e9062
2019-11-01 11:06:20 -04:00
Jon Atack
ff22751417
test: rm ascii art in rpc_fundrawtransaction
Doc changes only to test/functional/rpc_fundrawtransaction.py:

- remove ascii art or convert to a docstring when sufficiently different from
the logging

- touch up other comments while here
2019-11-01 11:56:53 +01:00
Jon Atack
94fcc08541
test: add rpc_fundrawtransaction logging
test/functional/rpc_fundrawtransaction.py is fairly long to run and has no
logging, so it can appear to be stalled.

This commit adds info logging at each test to provide feedback on the test run.
2019-11-01 11:27:11 +01:00
MarcoFalke
222b7d0ca7
Merge #17330: test: Add shrinkdebugfile=0 to regtest bitcoin.conf
c5377ffbbb [qa] Add shrinkdebugfile=0 to regtest bitcoin.conf (Suhas Daftuar)

Pull request description:

  This helps avoid accidentally truncating the debug.log while manually
  debugging.

ACKs for top commit:
  MarcoFalke:
    ACK c5377ffbbb
  dongcarl:
    ACK c5377ffbbb

Tree-SHA512: a05d6a7c494ee2c300fa1a9dbaa48b7fed63a44b1fa823198cbf401cff1c4aa8e44eb431fa635b27a1987d0eb1a5f8e2712514dcea7c5dd05240a445c8bd505d
2019-10-31 14:57:44 -04:00
Suhas Daftuar
c5377ffbbb [qa] Add shrinkdebugfile=0 to regtest bitcoin.conf
This helps avoid accidentally truncating the debug.log while manually
debugging.
2019-10-31 13:54:07 -04:00
John Newbery
60582d6060 [linter] Strip trailing / in path for git-subtree-check
git-subtree-check fails if the directory is given with a trailing slash,
eg:

```
> test/lint/git-subtree-check.sh src/univalue/
ERROR: src/univalue/ is not a subtree
```

Shell autocompletes will add the trailing slash when autofilling the
path name, which will therefore cause the script to fail.

Just ignore any trailing slash.
2019-10-31 11:53:46 -04:00
Luke Dashjr
a6f6f77a86 QA: Add wallet_implicitsegwit to test the ability to transform keys between address types 2019-10-30 18:56:25 +00:00
Adam Jonas
c98bd13e67 replace asserts in RPC code with CHECK_NONFATAL and add linter 2019-10-30 12:03:07 -04:00
Wladimir J. van der Laan
3c40bc6726
Merge #15921: validation: Tidy up ValidationState interface
3004d5a12d [validation] Remove fMissingInputs from AcceptToMemoryPool() (John Newbery)
c428622a5b [validation] Remove unused first_invalid parameter from ProcessNewBlockHeaders() (John Newbery)
7204c6434b [validation] Remove useless ret parameter from Invalid() (John Newbery)
1a37de4b31 [validation] Remove error() calls from Invalid() calls (John Newbery)
067981e492 [validation] Tidy Up ValidationResult class (John Newbery)
a27a2957ed [validation] Add CValidationState subclasses (John Newbery)

Pull request description:

  Carries out some remaining tidy-ups remaining after PR 15141:

  - split ValidationState into TxValidationState and BlockValidationState (commit from ajtowns)
  - various minor code style tidy-ups to the ValidationState class
  - remove the useless `ret` parameter from `ValidationState::Invalid()`
  - remove the now unused `first_invalid` parameter from `ProcessNewBlockHeaders()`
  - remove the `fMissingInputs` parameter from `AcceptToMemoryPool()`, and deal with missing inputs the same way as other errors by using the `TxValidationState` object.

  Tip for reviewers (thanks ryanofsky!): The first commit ("[validation] Add CValidationState subclasses" ) is huge and can be easier to start reviewing if you revert the rote, mechanical changes:

  Substitute the commit hash of commit "[validation] Add CValidationState subclasses" for <CommitHash> in the commands below.

  ```sh
  git checkout <CommitHash>
  git grep -l ValidationState | xargs sed -i 's/BlockValidationState\|TxValidationState/CValidationState/g'
  git grep -l ValidationResult | xargs sed -i 's/BlockValidationResult\|TxValidationResult/ValidationInvalidReason/g'
  git grep -l MaybePunish | xargs sed -i 's/MaybePunishNode\(ForBlock\|ForTx\)/MaybePunishNode/g'
  git diff HEAD^
  ```

  After that it's possible to easily see the mechanical changes with:

  ```sh
  git log -p -n1 -U0 --word-diff-regex=. <CommitHash>
  ```

ACKs for top commit:
  laanwj:
    ACK 3004d5a12d
  amitiuttarwar:
    code review ACK 3004d5a12d. Also built & ran tests locally.
  fjahr:
    Code review ACK 3004d5a12d . Only nit style change and pure virtual destructor added since my last review.
  ryanofsky:
    Code review ACK 3004d5a12d. Just whitespace change and pure virtual destructor added since last review.

Tree-SHA512: 511de1fb380a18bec1944ea82b513b6192df632ee08bb16344a2df3c40811a88f3872f04df24bc93a41643c96c48f376a04551840fd804a961490d6c702c3d36
2019-10-30 15:37:34 +01:00
MarcoFalke
fa144e6fde
rpc: Add generatetodescriptor 2019-10-30 10:01:32 -04:00
John Newbery
3004d5a12d [validation] Remove fMissingInputs from AcceptToMemoryPool()
Handle this failure in the same way as all other failures: call Invalid()
with the reasons for the failure.
2019-10-29 15:46:45 -04:00
MarcoFalke
6a97e8a060
Merge #17260: Split some CWallet functions into new LegacyScriptPubKeyMan
f201ba59ff Refactor: Split up CWallet and LegacyScriptPubKeyMan and classes (Andrew Chow)
6702048f91 MOVEONLY: Move key handling code out of wallet to keyman file (Andrew Chow)
ab053ec6d1 Move wallet enums to walletutil.h (Andrew Chow)

Pull request description:

  Moves key management functions into a new class LegacyScriptPubKeyMan. First two commits are move-only commits which move stuff out of wallet.{h/cpp} and into newly created scriptpubkeyman.{h/cpp}. Third commit changes several things in CWallet to use LegacyScriptPubKeyMan.

  First step in the wallet boxes refactor. Note that LegacyScriptPubKeyMan and ScriptPubKeyMan cannot be used standalone yet and are still very much tied into CWallet with both accessing functions within each other. This PR is to help reduce review burden.

ACKs for top commit:
  Sjors:
    Code review ACK f201ba5.
  promag:
    Code review ACK f201ba59ff.
  ryanofsky:
    Code review ACK f201ba59ff
  MarcoFalke:
    ACK f201ba59ff

Tree-SHA512: bdc0d8595a06233fe003afcf968a38e0e8cc584a6a89c5bcd05309ac29dca852391802d46763ef81a108d146d0f40c79ea5438e87234ed12b4b8360c9aec94c0
2019-10-29 08:19:23 -04:00
Adam Jonas
436ad43643 Fix issue with conflicted mempool tx in listsinceblock
listsinceblock now checks that returned transactions are not
conflicting with any transactions that are filtered out by
the given blockhash

Co-Authored-By: Michael Chrostowski <michael.chrostowski@gmail.com>
2019-10-28 10:26:46 -04:00
fanquake
badca85e2c
Merge #16202: p2p: Refactor network message deserialization
ed2dc5e48a Add override/final modifiers to V1TransportDeserializer (Pieter Wuille)
f342a5e61a Make resetting implicit in TransportDeserializer::Read() (Pieter Wuille)
6a91499496 Remove oversized message detection from log and interface (Pieter Wuille)
b0e10ff4df Force CNetMessage::m_recv to use std::move (Jonas Schnelli)
efecb74677 Use adapter pattern for the network deserializer (Jonas Schnelli)
1a5c656c31 Remove transport protocol knowhow from CNetMessage / net processing (Jonas Schnelli)
6294ecdb8b Refactor: split network transport deserializing from message container (Jonas Schnelli)

Pull request description:

  **This refactors the network message deserialization.**

  * It transforms the `CNetMessage` into a transport protocol agnostic message container.
  * A new class `TransportDeserializer` (unique pointer of `CNode`)  is introduced, handling the network buffer reading and the decomposing to a `CNetMessage`
  * **No behavioral changes** (in terms of disconnecting, punishing)
  * Moves the checksum finalizing into the `SocketHandler` thread (finalizing was in `ProcessMessages` before)

  The **optional last commit** makes the `TransportDeserializer` following an adapter pattern (polymorphic interface) to make it easier to later add a V2 transport protocol deserializer.

  Intentionally not touching the sending part.

  Pre-Requirement for BIP324 (v2 message transport protocol).
  Replacement for #14046 and inspired by a [comment](https://github.com/bitcoin/bitcoin/pull/14046#issuecomment-431528330) from sipa

ACKs for top commit:
  promag:
    Code review ACK ed2dc5e48a.
  marcinja:
    Code review ACK ed2dc5e48a
  ryanofsky:
    Code review ACK ed2dc5e48a. 4 cleanup commits added since last review. Unaddressed comments:
  ariard:
    Code review and tested ACK ed2dc5e.

Tree-SHA512: bab8d87464e2e8742529e488ddcdc8650f0c2025c9130913df00a0b17ecdb9a525061cbbbd0de0251b76bf75a8edb72e3ad0dbf5b79e26f2ad05d61b4e4ded6d
2019-10-28 09:15:59 -04:00
Wladimir J. van der Laan
9ae468a6d5
Merge #17192: util: Add CHECK_NONFATAL and use it in src/rpc
faeb666536 util: Add CHECK_NONFATAL and use it in src/rpc (MarcoFalke)

Pull request description:

  Fixes #17181

  Currently, we use `assert` in RPC code to document logic and code assumptions. However, it seems a bit extreme to abort all of Bitcoin Core on an assert failure in one of the RPC threads. I suggest to replace all `assert`s with a macro `CHECK_NONFATAL(condition)` that throws a runtime error when the condition evaluates to `false`. That runtime error will then be returned to the rpc caller and will include instructions to report the error to our issue tracker.

ACKs for top commit:
  practicalswift:
    ACK faeb666536
  laanwj:
    ACK faeb666536
  ryanofsky:
    Code review ACK faeb666536

Tree-SHA512: 9b748715a5e0767ac11f1324a95a3a6ec672a0e0658013492219223bda83ce4b1b447fd8183bbb235f7df5ef7dddda7666ad569544b4d61cc65f232ca7a800ec
2019-10-28 12:00:36 +01:00
Andrew Chow
f201ba59ff Refactor: Split up CWallet and LegacyScriptPubKeyMan and classes
This moves CWallet members and methods dealing with keys to a new
LegacyScriptPubKeyMan class, and updates calling code to reference the new
class instead of CWallet.

Most of the changes are simple text replacements and variable substitutions
easily verified with:

    git log -p -n1 -U0 --word-diff-regex=.

The only nontrivial chunk of code added is the new LegacyScriptPubKeyMan class
declaration, but this code isn't new and is just selectively copied and moved
from the previous CWallet class declaration. This can be verified with:

    git log -p -n1 --color-moved=dimmed_zebra src/wallet/scriptpubkeyman.h src/wallet/wallet.h

or

    git diff HEAD~1:src/wallet/wallet.h HEAD:src/wallet/scriptpubkeyman.h

This commit does not change behavior.
2019-10-25 19:20:24 -04:00
Wladimir J. van der Laan
b688b859db
Merge #17004: validation: Remove REJECT code from CValidationState
9075d13153 [docs] Add release notes for removal of REJECT reasons (John Newbery)
04a2f326ec [validation] Fix REJECT message comments (John Newbery)
e9d5a59e34 [validation] Remove REJECT code from CValidationState (John Newbery)
0053e16714 [logging] Don't log REJECT code when transaction is rejected (John Newbery)
a1a07cfe99 [validation] Fix peer punishment for bad blocks (John Newbery)

Pull request description:

  We no longer send BIP 61 REJECT messages, so there's no need to set
  a REJECT code in the CValidationState object.

  Note that there is a minor bug fix in p2p behaviour here. Because the
  call to `MaybePunishNode()` in `PeerLogicValidation::BlockChecked()` only
  previously happened if the REJECT code was > 0 and < `REJECT_INTERNAL`,
  then there are cases were `MaybePunishNode()` can get called where it
  wasn't previously:

  - when `AcceptBlockHeader()` fails with `CACHED_INVALID`.
  - when `AcceptBlockHeader()` fails with `BLOCK_MISSING_PREV`.

  Note that `BlockChecked()` cannot fail with an 'internal' reject code. The
  only internal reject code was `REJECT_HIGHFEE`, which was only set in
  ATMP.

  This reverts a minor bug introduced in 5d08c9c579.

ACKs for top commit:
  ariard:
    ACK 9075d13, changes since last reviewed are splitting them in separate commits to ease understanding and fix nits
  fjahr:
    ACK 9075d13153, confirmed diff to last review was fixing nits in docs/comments.
  ryanofsky:
    Code review ACK 9075d13153. Only changes since last review are splitting the main commit and updating comments

Tree-SHA512: 58e8a1a4d4e6f156da5d29fb6ad6a62fc9c594bbfc6432b3252e962d0e9e10149bf3035185dc5320c46c09f3e49662bc2973ec759679c0f3412232087cb8a3a7
2019-10-24 10:49:45 +02:00
Wladimir J. van der Laan
4258fd7377
Merge #17091: tests: Add test for loadblock option and linearize scripts
89339d1460 tests: Add test for loadblock option (Fabian Jahr)

Pull request description:

  Fixes #17019

  Was initially part of #17044 but as the test got larger it made sense to split it into its own commit as suggested in #17019 .

  This is testing the `-loadblock` option by using the scripts in `contrib/linearize` to generate a `bootstrap.dat` file and starting a disconnected node with it. So it is also testing the linearize scripts which were untested before and needed to be made available for the CI environment, hence they are added to `DIST_CONTRIB` in `Makefile.am`.

ACKs for top commit:
  laanwj:
    ACK 89339d1460

Tree-SHA512: aede0cd6e8b21194973f3633bc07fa2672d66a6f85dfe6a57cee2bb269a65d19ea49d5f9ed7914a173b3847c76e70257aa865f44bde170c1999d9655b4862d1c
2019-10-23 11:21:46 +02:00
Pieter Wuille
6a91499496
Remove oversized message detection from log and interface 2019-10-23 09:27:25 +02:00
Sebastian Falbesoner
8d8e5a79d0 test: use default address type (bech32) for wallet_bumpfee tests
The use of native segwit addresses (pure p2wpkh instead of p2sh-p2wpkh) leads
to smaller transaction sizes, needing adaption of some constants in the
following test cases:
    - test_dust_to_fee(): adaption of dust calculation
          (p2wpkh spend estimate of 67 is taken from src/policy/policy.cpp:GetDustThreshold())
    - test_maxtxfee_fails(): lowering -maxtxfee setting to trigger fail
2019-10-22 16:38:48 +02:00
Jeremy Rubin
4671fc3d9e Expand on wallet_balance.py comment from https://github.com/bitcoin/bitcoin/pull/16766\#issuecomment-527563982 2019-10-21 13:43:44 -07:00
Jeremy Rubin
5ffe0d1449 Update comment in test/functional/wallet_balance.py
Co-Authored-By: MarcoFalke <falke.marco@gmail.com>
2019-10-21 13:16:22 -07:00
Jeremy Rubin
a550c58267 Update wallet_balance.py test to reflect new behavior 2019-10-21 13:16:22 -07:00
practicalswift
0616138a07 tests: Remove no longer needed UBSan suppressions (issues fixed). Add documentation. 2019-10-21 18:24:27 +00:00
Wladimir J. van der Laan
a22b62481a
Merge #17070: wallet: Avoid showing GUI popups on RPC errors
facec1c643 wallet: Avoid showing GUI popups on RPC errors (MarcoFalke)

Pull request description:

  RPC errors and warnings are shown as popups in the GUI instead of being returned to the RPC caller. For example,

  ```
  $ ./src/bitcoin-cli loadwallet $(pwd)/./test/functional/data/wallets/high_minversion/
  error code: -4
  error message:
  Wallet loading failed.
  ```

  gives me a GUI popup and no reason why loading the wallet failed.

  After this pull request:

  ```
  $ ./src/bitcoin-cli loadwallet $(pwd)/./test/functional/data/wallets/high_minversion/
  error code: -4
  error message:
  Wallet loading failed: Error loading /home/marco/workspace/btc_bitcoin_core/./test/functional/data/wallets/high_minversion/wallet.dat: Wallet requires newer version of Bitcoin Core

ACKs for top commit:
  laanwj:
    Code review ACK facec1c643

Tree-SHA512: c8274bbb02cfcf71676eeec1e773e51fb3538cf93f82e7cb8536f4716d44ed819cdc162dfc039ac7386a4db381a734cdb27fd32567043a1180c02519fbcba194
2019-10-21 13:48:27 +02:00
Wladimir J. van der Laan
fc1040acc0
Merge #17176: ci: Cleanup macOS runs
fa677d1801 ci: Remove redundant check for TRAVIS_OS_NAME (MarcoFalke)
fadccb263b doc: Document that GNU tools are required for linters (MarcoFalke)
4444704ca9 ci: Cleanup macOS runs (MarcoFalke)

Pull request description:

  * Remove a commented out cleanup task in `before_cache`
  * Remove the linter run on macOS, and document that GNU tools are required to run the linters

ACKs for top commit:
  Sjors:
    Code review ACK fa677d1801
  laanwj:
    ACK fa677d1801
  ryanofsky:
    Code review ACK fa677d1801 for new third commit replacing TRAVIS_OS_NAME check with NO_DEPENDS setting

Tree-SHA512: 9122a63bbe7887d9e379123152ea4ba44324cb18033b9e6b45bfdb1af665c10ea598564b9fcd57330d208a08e4696e41b4d6175f05f0843a3a76530da114f8c6
2019-10-21 12:20:47 +02:00
MarcoFalke
faeb666536
util: Add CHECK_NONFATAL and use it in src/rpc 2019-10-18 17:19:36 -04:00
Jonas Schnelli
6294ecdb8b
Refactor: split network transport deserializing from message container 2019-10-18 08:56:06 +02:00
MarcoFalke
88eff969c2
Merge #17177: doc: Describe log files + consistent paths in test READMEs
9576614d2d doc: Describe log files + consistent paths in test READMEs (Martin Erlandsson)

Pull request description:

  picks up #15830

  I saw this was almost ready to merge but the test logging part was not 100% correct. I reworked that part, the rest is the same.

ACKs for top commit:
  GChuf:
    ACK 9576614d2d

Tree-SHA512: 3de7f1b0a1b0419df6e7b55964d00e715b6cb7874b1849ad6f120597610d7df4182c4b61b9c9691ce04f4e392ed3caead4c623374be2066ac31319e702d45d09
2019-10-17 14:35:21 -04:00
Martin Erlandsson
9576614d2d doc: Describe log files + consistent paths in test READMEs 2019-10-17 17:53:45 +02:00
MarcoFalke
fadccb263b
doc: Document that GNU tools are required for linters 2019-10-17 10:52:11 -04:00