Commit graph

1926 commits

Author SHA1 Message Date
Jon Atack
acc14c5093
test: fix incorrect value in rpc_rawtransaction.py 2019-09-21 15:55:51 +02:00
MarcoFalke
f8b0b190aa
Merge #16920: test: Fix extra_args in wallet_import_rescan.py
fa2e038691 test: Fix extra_args in wallet_import_rescan.py (MarcoFalke)

Pull request description:

  Bug introduced by me (🤦‍♂️) in fa25668e1c

  For reference:

  ```
  >>> a = [[]]*2
  >>> a[0] += ['ONE']
  >>> a
  [['ONE'], ['ONE']]

  >>> a = [[] for _ in range(2)]
  >>> a[0] += ['ONE']
  >>> a
  [['ONE'], []]

ACKs for top commit:
  theStack:
    utACK fa2e038

Tree-SHA512: 7d75a0d06233d013d62198ea95793612242254d5d90f393d01b2beef5abc78d6e85c796532311638f16cfed3b66a7ae41a108c0fe6f0f5d7f6616b042c670df7
2019-09-20 08:19:59 -04:00
fanquake
04321494ae
Merge #16921: tests: Add information on how to add Vulture suppressions
72a18a73af tests: Add information on how to add Vulture suppressions (practicalswift)

Pull request description:

  Add information on how to add `vulture` suppressions.

  As requested by MarcoFalke in https://github.com/bitcoin/bitcoin/issues/16906#issuecomment-533264107 -- your wish is my command! :)

ACKs for top commit:
  fanquake:
    ACK 72a18a73af - similar sort of message as in [lint-spelling.sh](https://github.com/bitcoin/bitcoin/blob/master/test/lint/lint-spelling.sh).

Tree-SHA512: b347f8cea33d4b0ba987a972979b0ac3423938084fea923a2c457a8081bc839a94ad818689d147477104b9197dc35be413f76a96026cd1507b4411d7513e3464
2019-09-20 17:07:38 +08:00
practicalswift
72a18a73af tests: Add information on how to add Vulture suppressions 2019-09-19 21:13:26 +00:00
MarcoFalke
fa2e038691
test: Fix extra_args in wallet_import_rescan.py 2019-09-19 15:25:00 -04:00
fridokus
96299a9d6c Test: Move common function assert_approx() into util.py 2019-09-19 14:53:40 +02:00
fanquake
9bf5768dd6
Merge #16885: doc: Update tx-size-small comment with relevant CVE disclosure
c4b0c08f7c Update tx-size-small comment with relevant CVE disclosure (Gregory Sanders)

Pull request description:

  Code first introduced under https://github.com/bitcoin/bitcoin/pull/11423 with essentially no description and no discussion.

ACKs for top commit:
  MarcoFalke:
    ACK c4b0c08f7c
  fanquake:
    ACK c4b0c08f7c

Tree-SHA512: 95d5c92998b8b1e944c477dbaee265b62612b6e815099ab31d9ff580b4dff777abaf7f326a284644709f918aa1510412d62310689b1250ef6e64de7b19ca9f71
2019-09-19 08:51:30 +08:00
Gregory Sanders
c4b0c08f7c Update tx-size-small comment with relevant CVE disclosure 2019-09-18 16:21:44 -04:00
MarcoFalke
59c138d2f1
Merge #16898: test: Remove connect_nodes_bi
fadfd844de test: Remove unused connect_nodes_bi (MarcoFalke)
fa3b9ee8b2 scripted-diff: test: Replace connect_nodes_bi with connect_nodes (MarcoFalke)
faaee1e39a test: Use connect_nodes when connecting nodes in the test_framework (MarcoFalke)
1111bb91f5 test: Reformat python imports to aid scripted diff (MarcoFalke)

Pull request description:

  By default all test nodes are connected in a chain. However, instead of just a single connection between each pair of nodes, we end up with up to four connections for a "middle" node (two outbound, two inbound, from each side).

  This is generally redundant (tx and block relay should succeed with just a single connection) and confusing. For example, test timeouts after a call to `sync_` may be racy and hard to reproduce. On top of that, the test `debug.log`s are hard to read because txs and block invs may be relayed on the same connection multiple times.

  Fix this by inlining `connect_nodes_bi` in the two tests that need it, and then replace it with a single `connect_nodes` in all other tests.

  Historic background:

  `connect_nodes_bi` has been introduced as a (temporary?) workaround for bug #5113 and #5138, which has long been fixed in #5157 and #5662.

ACKs for top commit:
  laanwj:
    ACK fadfd844de
  jonasschnelli:
    utACK fadfd844de - more of less a cleanup PR.
  promag:
    Tested ACK fadfd844de, ran extended tests.

Tree-SHA512: 2d027a8fd150749c071b64438a0a78ec922178628a7dbb89fd1212b0fa34febd451798c940101155d3617c0426c2c4865174147709894f1f1bb6cfa336aa7e24
2019-09-18 14:41:21 -04:00
MarcoFalke
cfcaa9759e
Merge #16907: test: lint: Add DisabledOpcodeTemplates to whitelist
fac35b21e2 test: lint: Add DisabledOpcodeTemplates to whitelist (MarcoFalke)

Pull request description:

  Fixes #16906

Top commit has no ACKs.

Tree-SHA512: 98b175bb062425fd3a8bd0d0258f4c0e0d5106980f1e037df7c2b2b2e5aa6031b11b582c026265d7b2de56049ccbadb0b7add9130d323f15886f681c6268ba0a
2019-09-18 12:27:02 -04:00
MarcoFalke
fac35b21e2
test: lint: Add DisabledOpcodeTemplates to whitelist
Also, bump vulture version to include the whitelist for threading module
2019-09-18 11:38:37 -04:00
Wladimir J. van der Laan
0ee0474234
Merge #16521: rpc: Use the default maxfeerate value as BTC/kB
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
2019-09-18 16:49:18 +02:00
Wladimir J. van der Laan
72d30d668a
Merge #16512: rpc: Shuffle inputs and outputs after joining psbts
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
2019-09-18 16:19:47 +02:00
Wladimir J. van der Laan
feb162d500
Merge #14696: qa: Add explicit references to related CVE's in p2p_invalid_block test.
0c62e3aa73 New regression testing for CVE-2018-17144, CVE-2012-2459, and CVE-2010-5137. (lucash-dev)
38bfca6bb2 Added comments referencing multiple CVEs in tests and production code. (lucash-dev)

Pull request description:

  This functional test includes two scenarios that test for regressions of vulnerabilities, but they are only briefly described. There are freely available documents explaining in detail the issues, but without explicit mentions, the developer trying to maintain the code needs an additional step of digging in commit history and PR conversations to figure it out.
  Added comments to explicitly mention  CVE-2018-17144 and CVE-2012-2459, for more complete documentation.
  This improves developer experience by making understanding the tests easier.

ACKs for top commit:
  laanwj:
    ACK 0c62e3aa73, checked the CVE numbers, thanks for adding documentation

Tree-SHA512: 3ee05351745193b8b959e4a25d50f25a693b2d24b0732ed53cf7d5882df40b5dd0f1877bd5c69cffb921d4a7acf9deb3cc1160b96dc730d9b5984151ad06b7c9
2019-09-18 16:00:54 +02:00
MarcoFalke
99beda47f5
Merge #16864: test: Add python bech32 impl round-trip test
ae0add8dfe Add python bech32 impl round-trip test (Gregory Sanders)

Pull request description:

  Currently there is a single use of `segwit_addr.encode`, and zero uses of `segwit_addr.decode` in the codebase.

  This adds a simple round-trip test of the implementation to avoid future regressions.

Top commit has no ACKs.

Tree-SHA512: feb3303f240f5987993e092ec15b878c8db3957d338db6a08fbe947bbfea0c558c7ebc26f8052c38a69d85c354f24e71431e19e0a2991c3c64b604f6d50697ff
2019-09-17 14:23:38 -04:00
MarcoFalke
318890b1ee
Merge #16888: test: Bump timeouts in slow running tests
fa502cb6f0 test: Bump timeouts in slow running tests (MarcoFalke)

Pull request description:

  Fixes #16794

ACKs for top commit:
  jamesob:
    ACK fa502cb6f0

Tree-SHA512: 52d1a6f9febe066332cc9df40638fdc3e8aaf1990caf912073b42f2f6615879da5512533ff71b85b4865034bc30da46945d34916669068e004e68058aeb04e90
2019-09-17 13:36:53 -04:00
MarcoFalke
fadfd844de
test: Remove unused connect_nodes_bi 2019-09-17 13:09:33 -04:00
MarcoFalke
fa3b9ee8b2
scripted-diff: test: Replace connect_nodes_bi with connect_nodes
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended -e 's/connect_nodes_bi\(self.nodes,\s*(.),\s*/connect_nodes(self.nodes[\1], /g' $(git grep -l connect_nodes_bi)
sed -i --regexp-extended -e 's/connect_nodes_bi(,| )/connect_nodes\1/g'                                  $(git grep -l connect_nodes_bi)
-END VERIFY SCRIPT-
2019-09-17 13:08:21 -04:00
MarcoFalke
faaee1e39a
test: Use connect_nodes when connecting nodes in the test_framework 2019-09-17 13:08:17 -04:00
MarcoFalke
1111bb91f5
test: Reformat python imports to aid scripted diff 2019-09-17 12:56:56 -04:00
MarcoFalke
fa502cb6f0
test: Bump timeouts in slow running tests 2019-09-16 13:32:13 -04:00
MarcoFalke
796b713633
Merge #16845: test: Add notes on how to generate data/wallets/high_minversion
2222c96dee test: Add notes on how to generate data/wallets/high_minversion (MarcoFalke)

Pull request description:

  I forgot to do this in #16796

ACKs for top commit:
  ryanofsky:
    ACK 2222c96dee

Tree-SHA512: 5f24ffa641b97eac4febad42ade7228b14fa72335c918a10880c5dec86a3ecc3075a31526f275188e07fea95b8e2c6320c64f716099f604b00e13d5366fcee37
2019-09-16 13:04:56 -04:00
MarcoFalke
2222c96dee
test: Add notes on how to generate data/wallets/high_minversion 2019-09-16 09:35:05 -04:00
Wladimir J. van der Laan
cd737214ce
Merge #16737: test: Establish only one connection between nodes in rpc_invalidateblock
fae961de6b test: Establish only one connection between nodes in rpc_invalidateblock (MarcoFalke)

Pull request description:

  Headers and block sync should eventually converge to the same result, regardless of whether the peers treat each other as "inbound" or "outbound".

  `connect_nodes_bi` has been introduced as a (temporary?) workaround for bug #5113 and #5138, which has long been fixed in #5157 and #5662.

  Thus remove the `connect_nodes_bi` workaround from the rpc_invalidateblock test.

  Conveniently, this also closes #16453. See https://github.com/bitcoin/bitcoin/issues/16444#issuecomment-514801708 for rationale

ACKs for top commit:
  laanwj:
    ACK fae961de6b

Tree-SHA512: b3614c66a205823df73f64d19cacfbec269beb5db52ff79004d746e17d7c0dfb43ab9785fdddc97e2a76fe76286c8c605b34df3dda4a2bf5be035f01169ae89a
2019-09-16 13:46:39 +02:00
Gregory Sanders
ae0add8dfe Add python bech32 impl round-trip test 2019-09-15 15:22:22 -04:00
Jon Atack
1b41c2c8a1
test: improve gettransaction test coverage
- Test gettransaction response without verbose, with verbose=False, and with verbose=True.

- In each case, test presence of expected fields in the output, including absence of the "decoded" field when `verbose` is not passed or false.

- Test that the "details" field contains the expected receive vout in each case.
2019-09-15 18:53:16 +02:00
Jon Atack
0f34f54888
rpc: fix regression in gettransaction
PR 16866 renamed the 'decode' argument in gettransaction to 'verbose' to make it more consistent with other RPC calls like getrawtransaction.

However, it seems it inadvertently overloaded the 'details' fields when 'verbose' is passed. The result is that the original 'details' fields are no longer returned, which seems to be a breaking API change.

This PR takes the simplest path to restoring the 'details' fields by renaming them from 'details' back to 'decoded', while leaving the 'verbose' argument for API consistency.

It also addresses [this comment](https://github.com/bitcoin/bitcoin/pull/16185#discussion_r320740413) to mention that the 'decoded' field is identical to decoderawtransaction.

Update the RPC help, functional test, and release note.
2019-09-14 20:17:19 +02:00
John Newbery
7dee8f4808 [wallet] Rename 'decode' argument in gettransaction method to 'verbose'
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.
2019-09-13 22:33:46 +03:00
MarcoFalke
ca97d292ce
Merge #16551: test: Test that low difficulty chain fork is rejected
333317ce6b test: Test that low difficulty chain fork is rejected (MarcoFalke)
fa31dc1bf4 test: Pass down correct chain name in tests (MarcoFalke)

Pull request description:

  To prevent OOM, Bitcoin Core will reject chain forks at low difficulty by default. This is the only use-case of checkpoints, so add a test for it to make sure the feature works as expected. If it didn't work, checkpoints would have no use-case and we might as well remove them

ACKs for top commit:
  Sjors:
    Thanks for adding the node 1 example. Code review ACK 333317c

Tree-SHA512: 90dffa540d0904f3cffb61d2382b1a26f84fe9560b7013e4461546383add31a8757b350616a6d43217c59ef7b8b2a1b62bb3bab582c679cbb2c660a782ce7be1
2019-09-12 13:28:49 +03:00
Wladimir J. van der Laan
04d9939f46
Merge #16850: test: servicesnames field in getpeerinfo and getnetworkinfo
1d524c62ea tests: rename 'test_getnetworkinginfo' in 'test_getnetworkinfo' (darosior)
07a8f65031 tests: add a test for the 'servicesnames' RPC field (darosior)

Pull request description:

  As per https://github.com/bitcoin/bitcoin/pull/16787#issuecomment-529801457, fixes #16844.

  This adds a test for both commands in the first commit and renames the test for `getnetworkinfo` in the second commit.

ACKs for top commit:
  laanwj:
    ACK 1d524c62ea

Tree-SHA512: 8267dce4d54356debab75014e6f9ba885b892da605ed32f26a5446c232992fcae761861bb678adbdb942815d4706f3768c70deee6afec68f219b23605475be01
2019-09-12 09:24:16 +02:00
darosior
1d524c62ea
tests: rename 'test_getnetworkinginfo' in 'test_getnetworkinfo'
Since it's the name of the RPC call
2019-09-11 17:26:29 +02:00
darosior
07a8f65031
tests: add a test for the 'servicesnames' RPC field
In getpeerinfo and getnetworkinfo
2019-09-11 17:25:53 +02:00
fanquake
2296fe65f5
Merge #16251: Improve signrawtransaction error reporting
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: #13218
  Fixes: #14823

ACKs for top commit:
  instagibbs:
    utACK ec4c79326b
  achow101:
    Code Review ACK ec4c79326b
  meshcollider:
    utACK ec4c79326b

Tree-SHA512: 0c95c91d498e85b834662b9e5c83f336ed5fd306be7701ce1dbfa0836fbeb448a267a796585512f7496e820be668b07c2a0a2f45e52dc23f09ee7d9c87e42b35
2019-09-11 15:37:13 +10:00
Anthony Towns
3c481f8921 signrawtransactionwithkey: better error messages for bad redeemScript/witnessScript
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.
2019-09-10 15:41:50 +10:00
Samuel Dobson
335b34c30c
Merge #16725: Don't show addresses or P2PK in decoderawtransaction
6d803494b5 Don't show addresses or P2PK in decoderawtransaction (nicolas.dorier)

Pull request description:

  I spent significant amount of time explaining to people that satoshi did not had any "bitcoin address", because bitcoin address was not existing at the time.

  Then I need to explain them that all blockchain explorer are wrong. Then I understood that the source of this widespread mistake come from Bitcoin Core itself.

  For:
  ```
  bitcoin-cli -regtest decoderawtransaction 01000000010000000000000000000000000000000000000000000000000000000000000000ffffffff4d04ffff001d0104455468652054696d65732030332f4a616e2f32303039204368616e63656c6c6f72206f6e206272696e6b206f66207365636f6e64206261696c6f757420666f722062616e6b73ffffffff0100f2052a01000000434104678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5fac00000000
  ```

  Before:
  ```json
  {
    "txid": "4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b",
    "hash": "4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b",
    "version": 1,
    "size": 204,
    "vsize": 204,
    "weight": 816,
    "locktime": 0,
    "vin": [
      {
        "coinbase": "04ffff001d0104455468652054696d65732030332f4a616e2f32303039204368616e63656c6c6f72206f6e206272696e6b206f66207365636f6e64206261696c6f757420666f722062616e6b73",
        "sequence": 4294967295
      }
    ],
    "vout": [
      {
        "value": 50.00000000,
        "n": 0,
        "scriptPubKey": {
          "asm": "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f OP_CHECKSIG",
          "hex": "4104678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5fac",
          "reqSigs": 1,
          "type": "pubkey",
          "addresses": [
            "mpXwg4jMtRhuSpVq4xS3HFHmCmWp9NyGKt"
          ]
        }
      }
    ]
  }
  ```

  After
  ```json
  {
    "txid": "4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b",
    "hash": "4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b",
    "version": 1,
    "size": 204,
    "vsize": 204,
    "weight": 816,
    "locktime": 0,
    "vin": [
      {
        "coinbase": "04ffff001d0104455468652054696d65732030332f4a616e2f32303039204368616e63656c6c6f72206f6e206272696e6b206f66207365636f6e64206261696c6f757420666f722062616e6b73",
        "sequence": 4294967295
      }
    ],
    "vout": [
      {
        "value": 50.00000000,
        "n": 0,
        "scriptPubKey": {
          "asm": "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f OP_CHECKSIG",
          "hex": "4104678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5fac",
          "reqSigs": 1,
          "type": "pubkey",
          "addresses": [
          ]
        }
      }
    ]
  }
  ```

  This mistake is having widespread impact, as developer thinks P2PK are addresses, they start running into issues when somebody send a P2PK payment to them and then they don't understand why they can't sign it like a P2PKH.

ACKs for top commit:
  Sjors:
    Code review ACK 6d80349.
  MarcoFalke:
    ACK 6d803494b5
  meshcollider:
    utACK 6d803494b5
  kristapsk:
    ACK 6d803494b5 (applied changes except test, ran tests, then applied changes to test also)

Tree-SHA512: 6e4990164a6b8df6675f09b2b189b7197fad43f1918fc1a4530ebd98ce71c3c94d9ec54e1b4624210fd7c5200d4f04825ca37f4e42f5fe9b8a9c0f38c50591ef
2019-09-10 11:43:48 +12:00
Samuel Dobson
8af835a72d
Merge #16796: wallet: Fix segfault in CreateWalletFromFile
fa734603b7 wallet: Fix segmentation fault in CreateWalletFromFile (MarcoFalke)
fab3c34412 test: Print both messages on failure in assert_raises_message (MarcoFalke)
faa13539d5 wallet: Fix documentation around WalletParameterInteraction (MarcoFalke)

Pull request description:

  Comes with a test to aid review. The test should fail without the fix to bitcoind

  The following `CreateWalletFromFile` issues are fixed:

  * `walletFile` refers to freed memory and will thus corrupt the debug.log and/or crash the node if read
  * `WalletParameterInteraction` was moved to `CreateWalletFromFile` and `WalletInit::ParameterInteraction` without updating the documentation

ACKs for top commit:
  promag:
    ACK fa734603b7.
  darosior:
    ACK fa734603b7
  meshcollider:
    LGTM, code-read ACK fa734603b7

Tree-SHA512: 2aceb63a3f25b90a840cfa08d37f5874aad4eb3df8c2ebf94e2ed18b55809b185e6920bdb345b988bff1fcea5e68a214fe06c361f7da2c01a3cc29e0cc421cb4
2019-09-09 23:34:05 +12:00
Wladimir J. van der Laan
08bb4c3156
Merge #16285: rpc: Improve scantxoutset response and help message
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
2019-09-09 08:08:51 +02:00
João Barbosa
bdd6a4fd5d qa: Check scantxoutset result against gettxoutsetinfo 2019-09-09 00:51:31 +01:00
fanquake
189c19e012
Merge #15759: p2p: Add 2 outbound block-relay-only connections
0ba08020c9 Disconnect peers violating blocks-only mode (Suhas Daftuar)
937eba91e1 doc: improve comments relating to block-relay-only peers (Suhas Daftuar)
430f489027 Don't relay addr messages to block-relay-only peers (Suhas Daftuar)
3a5e885306 Add 2 outbound block-relay-only connections (Suhas Daftuar)
b83f51a4bb Add comment explaining intended use of m_tx_relay (Suhas Daftuar)
e75c39cd42 Check that tx_relay is initialized before access (Suhas Daftuar)
c4aa2ba822 [refactor] Change tx_relay structure to be unique_ptr (Suhas Daftuar)
4de0dbac9b [refactor] Move tx relay state to separate structure (Suhas Daftuar)
26a93bce29 Remove unused variable (Suhas Daftuar)

Pull request description:

  Transaction relay is optimized for a combination of redundancy/robustness as well as bandwidth minimization -- as a result transaction relay leaks information that adversaries can use to infer the network topology.

  Network topology is better kept private for (at least) two reasons:

  (a) Knowledge of the network graph can make it easier to find the source IP of a given transaction.

  (b) Knowledge of the network graph could be used to split a target node or nodes from the honest network (eg by knowing which peers to attack in order to achieve a network split).

  We can eliminate the risks of (b) by separating block relay from transaction relay; inferring network connectivity from the relay of blocks/block headers is much more expensive for an adversary.

  After this commit, bitcoind will make 2 additional outbound connections that are only used for block relay. (In the future, we might consider rotating our transaction-relay peers to help limit the effects of (a).)

ACKs for top commit:
  sipa:
    ACK 0ba08020c9
  ajtowns:
    ACK 0ba08020c9 -- code review, ran tests. ran it on mainnet for a couple of days with MAX_BLOCKS_ONLY_CONNECTIONS upped from 2 to 16 and didn't observe any unexpected behaviour: it disconnected a couple of peers that tried sending inv's, and it successfully did compact block relay with some block relay peers.
  TheBlueMatt:
    re-utACK 0ba08020c9. Pointed out that stats.fRelayTxes was sometimes uninitialized for blocksonly peers (though its not a big deal and only effects RPC), which has since been fixed here. Otherwise changes are pretty trivial so looks good.
  jnewbery:
    utACK 0ba08020c9
  jamesob:
    ACK 0ba08020c9

Tree-SHA512: 4c3629434472c7dd4125253417b1be41967a508c3cfec8af5a34cad685464fbebbb6558f0f8f5c0d4463e3ffa4fa3aabd58247692cb9ab8395f4993078b9bcdf
2019-09-07 17:45:03 +08:00
fanquake
0d20c42a01
Merge #16421: Conservatively accept RBF bumps bumping one tx at the package limits
5ce822efbe Conservatively accept RBF bumps bumping one tx at the package limits (Matt Corallo)

Pull request description:

  Based on #15681, this adds support for some simple cases of RBF inside of large packages. Issue pointed out by sdaftuar in #15681, and this fix (or a broader one) is required ot make #15681 fully useful.

  Accept RBF bumps of single transactions (ie which evict exactly one
  transaction) even when that transaction is a member of a package
  which is currently at the package limit iff the new transaction
  does not add any additional mempool dependencies from the original.

  This could be made a bit looser in the future and still be safe,
  but for now this fixes the case that a transaction which was
  accepted by the carve-out rule will not be directly RBF'able

ACKs for top commit:
  instagibbs:
    re-ACK 5ce822efbe
  ajtowns:
    ACK 5ce822efbe ; GetSizeWithDescendants is only change and makes sense
  sipa:
    Code review ACK 5ce822efbe. I haven't thought hard about the effect on potential DoS issues this policy change may have.

Tree-SHA512: 1cee3bc57393940a30206679eb60c3ec8cb4f4825d27d40d1f062c86bd22542dd5944fa5567601c74c8d9fd425333ed3e686195170925cfc68777e861844bd55
2019-09-07 10:15:43 +08:00
MeshCollider
5e202382a9
Merge #16624: wallet: encapsulate transactions state
442a87cc0a Add a test wallet_reorgsrestore (Antoine Riard)
40ede992d9 Modify wallet tx status if has been reorged out (Antoine Riard)
7e89994133 Remove SyncTransaction for conflicted txn in CWallet::BlockConnected (Antoine Riard)
a31be09bfd Encapsulate tx status in a Confirmation struct (Antoine Riard)

Pull request description:

  While working on #15931, I've tried to rationalize tx state management to ease integration of block height tracking per-wallet tx. We currently rely on a combination of `hashBlock` and `nIndex` with magic value to determine tx confirmation, conflicted or abandoned state. It's hard to reason and error-prone.  To solve that, we encapsulate these fields in a `TxConfirmation` struct and introduce a `TxState` member that we update accordingly at block connection/disconnection.

  Following jnewbery [recommendation](https://github.com/bitcoin/bitcoin/pull/15931#discussion_r312576506), I've taken these changes in its own commit, and open a PR to get them first. It would ease review of aforementioned PR, but above all should ease fixing of long-term issues like :
  * https://github.com/bitcoin/bitcoin/issues/7315 (but maybe we should abandon abandontransaction or relieve it to only free outpoints not track the transaction as abandoned in itself, need its own discussion)
  * https://github.com/bitcoin/bitcoin/issues/8692 where we should cancel conflicted state of transactions chain smoothly
  * `MarkConflicted` in `LoadToWallet` is likely useless if we track conflicts rights at block connection

  Main changes of this PR to get right are tx update in `AddToWallet` and serialization/deserialization logic.

ACKs for top commit:
  meshcollider:
    Light re-Code Review ACK 442a87cc0a
  ryanofsky:
    utACK 442a87cc0a. Changes since last review are switching from `hasChain` to `LockChain` and removing chain lock in `WalletBatch::LoadWallet` that's redundant with the new lock still added in `CWallet::LoadWallet`, and fixing python test race condition.

Tree-SHA512: 029209e006de0240436817204e69e548c5665e2b0721b214510e7aba7eba130a5eab441d3a1ad95bd6426114dd27390492c77bf4560a9610009b32cd0a1f72f7
2019-09-06 01:28:54 +12:00
Wladimir J. van der Laan
cbde2bc806
Merge #16804: test: Remove unused try-block in assert_debug_log
fae91a09c4 test: Remove incorrect and unused try-block in assert_debug_log (MarcoFalke)

Pull request description:

  This try block has accidentally been added by me in fa3e9f7627.
  It was unused all the time, but commit 6011c9d72d added a `return` in the finally block, muting all exceptions.

  This can be tested by adding an `assert False` after any `with ...assert_debug_log...:` line.

ACKs for top commit:
  laanwj:
    ACK fae91a09c4
  ryanofsky:
    utACK fae91a09c4. I didn't know returning inside a `finally` block would cancel pending exceptions or return values, but I guess this makes sense and is a good thing to be aware of.

Tree-SHA512: 47ed0165062060e9af055a3e92f1a529cd41d00476bfad64e3cd141ae084d22f926a343bb1257717e164e15459a59ab66aed198c95d18bf780d8cb0b76aa3298
2019-09-05 13:28:51 +02:00
MarcoFalke
45be44cce4
Merge #15257: Scripts and tools: Bump flake8 to 3.7.8
3d0a82cff8 devtools: Accomodate block-style copyright blocks (Ben Woosley)
0ef0e51fe4 lint: Bump flake8 to 3.7.8 (Ben Woosley)
838920704a lint: Disable flake8 W504 warning (Ben Woosley)
b21680baf5 test/contrib: Fix invalid escapes in regex strings (Ben Woosley)

Pull request description:

  This is a second go at #15221, fixing new lints in:
  W504 line break after binary operator
  W605 invalid escape sequence
  F841 local variable 'e' is assigned to but never used

  This time around:
  * One commit per rule, for easier review
  * I went with the PEP-8 style of breaking before binary operators
  * I looked into the raw regex newline issue, and found that raw strings with newlines embedded do work appropriately. E.g. run `re.match(r" \n ", " \n ")` to check this for yourself. `re.MULTILINE` exists to modify `^` and `$` in multiline scenarios, but  all of these searches are per-line.

ACKs for top commit:
  practicalswift:
    ACK 3d0a82cff8 -- diff looks correct

Tree-SHA512: bea0c144cadd72e4adf2e9a4b4ee0535dd91a8e694206924cf8a389dc9253f364a717edfe9abda88108fbb67fda19b9e823f46822d7303c0aaa72e48909a6105
2019-09-05 02:43:13 +02:00
MarcoFalke
761fe07ba9
Merge #16768: test: Make lint-includes.sh work from any directory
490da639cb Make lint-includes.sh work from any directory (Kristaps Kaupe)

Pull request description:

  Before this change it works from root folder of bitcoin git repo, but if you do `cd test/lint; ./test-includes.sh`, you will have a lot of false positive messages like this:
  ```
  Good job! The circular dependency "chainparamsbase -> util/system -> chainparamsbase" is no longer present.
  Please remove it from EXPECTED_CIRCULAR_DEPENDENCIES in ./lint-circular-dependencies.sh
  to make sure this circular dependency is not accidentally reintroduced.

  Good job! The circular dependency "index/txindex -> validation -> index/txindex" is no longer present.
  Please remove it from EXPECTED_CIRCULAR_DEPENDENCIES in ./lint-circular-dependencies.sh
  to make sure this circular dependency is not accidentally reintroduced.

  ```

Top commit has no ACKs.

Tree-SHA512: 07fa69cb2883181dcee922191acac4b242722eeb2916cdffdc7163421302b22f3c9525aaf4c754a9dba1c307032c05285e38191d5c6aabc894321f8a27bbceaa
2019-09-05 02:00:44 +02:00
Andrew Chow
c0b5d97103 Test that joinpsbts randomly shuffles the inputs 2019-09-04 17:44:31 -04:00
Matt Corallo
5ce822efbe Conservatively accept RBF bumps bumping one tx at the package limits
Accept RBF bumps of single transactions (ie which conflict with one
transaction) even when that transaction is a member of a package
which is currently at the package limit iff the new transaction
does not add any additional mempool dependencies from the original.

This could be made a bit looser in the future and still be safe,
but for now this fixes the case that a transaction which was
accepted by the carve-out rule will not be directly RBF'able.
2019-09-04 15:53:14 -04:00
Kristaps Kaupe
490da639cb
Make lint-includes.sh work from any directory 2019-09-04 22:36:09 +03:00
Suhas Daftuar
0ba08020c9 Disconnect peers violating blocks-only mode
If we set fRelay=false in our VERSION message, and a peer sends an INV or TX
message anyway, disconnect. Since we use fRelay=false to minimize bandwidth,
we should not tolerate remaining connected to a peer violating the protocol.
2019-09-04 14:58:36 -04:00
MarcoFalke
fae91a09c4
test: Remove incorrect and unused try-block in assert_debug_log 2019-09-04 13:10:37 -04:00
Ben Woosley
838920704a
lint: Disable flake8 W504 warning
In the words of MarcoFalke:
"W504 should be disabled. This is not a critical error that should be blocking a merge"
https://github.com/bitcoin/bitcoin/pull/15257#discussion_r302017280
2019-09-03 14:40:56 -04:00