Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Be consistent in calling transactions "replaceable" for Opt-In RBF #10698

Merged
merged 3 commits into from Jul 6, 2017

Conversation

TheBlueMatt
Copy link
Contributor

@TheBlueMatt TheBlueMatt commented Jun 28, 2017

Thanks to @morcos and @luke-jr for pointing out that we were massively inconsistent between RPCs on IRC.

Note that none of these have been in a release, except "replaceable", which has been used in wallet tx descriptions in RPCs in 0.14.2, at least. Hence why I stuck with "replaceable" for all of them.

Also adds the missing named-argument option for createrawtransaction.

Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NACK, this breaks API compatibility with Knots for no reason. replacable also gives a false sense of other transactions not being replacable (which they are).

opt_into_rbf seems to be the sensible name, so use that and just provide backward compatibility aliases.

@TheBlueMatt
Copy link
Contributor Author

We do not have a framework for backwards compatibility aliases (but if you want to add one, feel free, dont think its worth doing for Knots, but we may need it at some point).

It is important to be consistent, and we use replaceable elsewhere in 0.14, so we should stick with that IMO, if folks want to paint the shed "opt_into_rbf", I'd be ok with that too.

@TheBlueMatt
Copy link
Contributor Author

This should likely be tagged 0.15.

@luke-jr
Copy link
Member

luke-jr commented Jun 28, 2017

Yes we do. See getblock's "verbosity" aka "verbose" param.

@TheBlueMatt
Copy link
Contributor Author

Ahh, didnt realize that, alright, well then its a super simple patch for Knots to carry to keep backwards compat with itself, no need to upstream it :).

@gmaxwell
Copy link
Contributor

in terms of the "other things arn't" -- this is why the list transaction output calls it BIP125 replaceable.

@luke-jr
Copy link
Member

luke-jr commented Jun 28, 2017

@TheBlueMatt The default should be compatibility. There is no reason to be incompatible.

@TheBlueMatt
Copy link
Contributor Author

@gmaxwell Good point, updated the docs to point to BIP 125.

@luke Indeed, compatibility is the default, but that does not override the legitimate reason that we should be consistent in our naming, and I'm really not at all convinced that compatibility with previous unreleased code is really much of a preference.

@luke-jr
Copy link
Member

luke-jr commented Jun 28, 2017

@TheBlueMatt Yes, we should be consistent, but compatibility doesn't interfere in this at all. It's not unreleased code; it's been part of Knots for several releases now.

@jonasschnelli
Copy link
Contributor

I generally think this change makes sense... though I'm worried about the API break.
Users/apps calling RPC with the then invalid optintorbf will not even get a notice that this has been deprecated/nuked, leading to the fact, that this could harm businesses.
Same could be true for the startup argument.

This is why I'm not sure if this is a "hard" improvement.
Ideally, we deprecate the old parameter first while we introduce the new one... but this is maybe an overkill.

Keeping it as it is right now seems for me the safest path (it's just wording).

If we go with this PR, then it would certainly require a release-notes part.

@@ -318,7 +318,8 @@ UniValue createrawtransaction(const JSONRPCRequest& request)
" ,...\n"
" }\n"
"3. locktime (numeric, optional, default=0) Raw locktime. Non-0 value also locktime-activates inputs\n"
"4. optintorbf (boolean, optional, default=false) Allow this transaction to be replaced by a transaction with higher fees. If provided, it is an error if explicit sequence numbers are incompatible.\n"
"4. replaceable (boolean, optional, default=false) Marks this transaction as BIP125 replaceable.\n"
" Allowing this transaction to be replaced by a transaction with higher fees. If provided, it is an error if explicit sequence numbers are incompatible.\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Allow or Allows is better than Allowing.

@@ -2655,7 +2655,8 @@ UniValue fundrawtransaction(const JSONRPCRequest& request)
" Those recipients will receive less bitcoins than you enter in their corresponding amount field.\n"
" If no outputs are specified here, the sender pays the fee.\n"
" [vout_index,...]\n"
" \"optIntoRbf\" (boolean, optional) Allow this transaction to be replaced by a transaction with higher fees\n"
" \"replaceable\" (boolean, optional) Marks this transaction as BIP125 replaceable.\n"
" Allowing this transaction to be replaced by a transaction with higher fees\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above - prefer Allow over Allowing.

@jnewbery
Copy link
Contributor

ACK.

though I'm worried about the API break.

This isn't an API break. These arguments are not yet in a release. We should change them to be consistent before v0.15 is released.

If we go with this PR, then it would certainly require a release-notes part.

Yes - since these are new arguments.

@laanwj
Copy link
Member

laanwj commented Jun 29, 2017

Concept ACK (we've discussed this on IRC)

Note that none of these have been in a release, except "replaceable", which has been used in wallet tx descriptions in RPCs in 0.14.2, at least. Hence why I stuck with "replaceable" for all of them.

This means that we're in time to fix this. Breaking compatiblity is a non-issue for things that haven't been in a release. Everything in master should be considered unstable from an API point of view.

it's been part of Knots for several releases now.

I'm sorry, but we really can't take into account the release schedule of derived projects. By taking on patches that aren't in a release yet, you risk later incompatibility.

@jonasschnelli
Copy link
Contributor

This means that we're in time to fix this. Breaking compatiblity is a non-issue for things that haven't been in a release. Everything in master should be considered unstable from an API point of view.

Ah. Yes. I though we have released the RBF opt-in argument in createrawtransaction and fundrawtransaction.
But right, we haven't so there is no API break. Sorry for the noise.

@luke-jr
Copy link
Member

luke-jr commented Jul 5, 2017

I'm sorry, but we really can't take into account the release schedule of derived projects. By taking on patches that aren't in a release yet, you risk later incompatibility.

If there was an argument for being incompatible, maybe, but when it is reasonable to be compatible, compatibility should be preferred. In this case, there is zero harm or overhead from being compatible, and opt_into_rbf is the clearer param name anyway.

NACK without that.

@jnewbery
Copy link
Contributor

jnewbery commented Jul 5, 2017

We do not have a framework for backwards compatibility aliases

Yes we do. See getblock's "verbosity" aka "verbose" param.

I wasn't aware of this, so I tried it out:

→ bitcoin-cli -named getblock blockhash=0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206 verbose=true
error code: -1
error message:
JSON value is not a boolean as expected

→ bitcoin-cli -named getblock blockhash=0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206 verbosity=true
{
  "hash": "0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206",
...

Seems like the implementation for getblock with verbose argument has been broken in bitcoin-cli since #8704. Should be an easy fix, but it's a subtle bug and a good example of why being tricksy with aliases and arguments is probably a bad idea.

@morcos
Copy link
Member

morcos commented Jul 5, 2017

utACK

To be consistent with other places (and add the missing named
args entry for it).
@TheBlueMatt TheBlueMatt force-pushed the 2017-06-replaceable-rpc-args branch from 8917ec6 to 73c942e Compare July 5, 2017 22:11
@TheBlueMatt
Copy link
Contributor Author

Changed "Allowing"s to "Allows" as requested by @jnewbery.

@laanwj
Copy link
Member

laanwj commented Jul 6, 2017

utACK 73c942e, this fixes one of the issues found by #10753.

@laanwj laanwj merged commit 73c942e into bitcoin:master Jul 6, 2017
laanwj added a commit that referenced this pull request Jul 6, 2017
… Opt-In RBF

73c942e Use "replaceable" instead of "rbfoptin" in bitcoin-tx. (Matt Corallo)
fb915d5 Use "replaceable" instead of "optIntoRbf" in fundrawtransaction. (Matt Corallo)
928c681 Use "replaceable" instead of "optintorbf" in createrawtransaction. (Matt Corallo)

Tree-SHA512: 8922451c00abb63aaa08b4a9e314e89c22233b32f207259fbc25367f7d5b67efbaccc7e2a4958c18611ad498da302296242860c7be965a0e996dcde3e89efa07
maflcko pushed a commit that referenced this pull request Aug 24, 2017
c6ec435 [tests] Add bitcoin_cli.py test script (John Newbery)
b23549f [tests] add TestNodeCLI class for calling bitcoin-cli for a node (John Newbery)

Pull request description:

  We don't test bitcoin-cli at all. That means that we can miss inconsistencies between the bitcoin-cli client and the RPC interface, such as #10698 and #10747. It also means that the various bitcoin-cli options and features are untested and regressions could be silently introduced.

  Let's fix that.

  This PR adds bitcoin-cli testing in the python functional test_framework:

  1. Add a bitcoin_cli.py test script that tests bitcoin-cli. At the moment it only tests that the result of `getinfo` is the same if you run it as an RPC or through bitcoin-cli, but can easily be extended to test additional bitcoin-cli features

  **EDIT: `--usecli` option is moved to a separate PR. This PR now only covers the bitcoin_cli.py test.**

  2. ~Add a `--usecli` option to the test framework. This changes the test to use bitcoin-cli for all RPC calls instead of using direct HTTP requests. This is somewhat experimental. It works for most tests, but there are some cases where it can't work transparently because:~
  - ~the testcase is asserting on a specific error code, and bitcoin-cli returns a different error code from the direct RPC~
  - ~we're sending a very large RPC request (eg `submitblock`) and it can't be serialized into a shell bitcoin-cli call.~

  ~I think that even though `--usecli` doesn't work on all tests, it's still a useful experimental feature. Future potential enhancements:~
  - ~enhance the framework to automatically skip tests that are known to fail with bitcoin-cli if the `--usecli` option is used.~
  - ~run a subset of tests in Travis with `-usecli`~

  This builds on and requires the `TestNode` PR #10711 . As an aside, this is a good demonstration of how tidy it is to add additional features/interfaces now that test node logic/state is encapsulated in a TestNode class.

  Addresses #10791

Tree-SHA512: a1e6be12e8e007f6f67b3d3bbcd142d835787300831eb38e6027a1ad25ca9d79c4bc99a41b19e31ee95205cba1b3b2d21a688b5909316aad70bfc2b4eb6d8a52
maflcko pushed a commit that referenced this pull request Sep 13, 2017
77aa9e5 test: Check RPC argument mapping (Wladimir J. van der Laan)

Pull request description:

  Parse the dispatch tables from the server implementation files, and the conversion table from the client (see #10751).

  Perform the following consistency checks:

  - Arguments defined in conversion table, must be present in dispatch table. If not, it was probably forgotten to add them to the dispatch table, and they will not work.

  - Arguments defined in conversion table must have the same names as in the dispatch table. If not, they will not work.

  - All aliases for an argument must either be present in the conversion table, or not. Anything in between means an oversight and some aliases won't work.

  Any of these results in an error.

  It also performs a consistency check to see if the same named argument is sometimes converted, and sometimes not. E.g. one RPC call might have a 'verbose' argument that is converted,
  another RPC call might have one that is not converted. This is not necessarily wrong, but points at a possible error (as well as makes the API harder to memorize) - so it is emitted as a warning (could upgrade this to error).

  This test is added to travis and run when `CHECK_DOC`. Currently fails with the following output:
  ```
  * Checking consistency between dispatch tables and vRPCConvertParams
  ERROR: createrawtransaction argument 3 (named optintorbf in vRPCConvertParams) is not defined in dispatch table
  ERROR: getblock argument ['verbosity', 'verbose'] has conflicts in vRPCConvertParams conversion specifier [True, False]
  WARNING: conversion mismatch for argument named verbose ([('getblock', False), ('getblockheader', True), ('getmempoolancestors', True), ('getmempooldescendants', True), ('getrawmempool', True), ('getrawtransaction', True)])
  ```
  - ~#10698 fixes the first ERROR~
  - #10747 fixes the second ERROR, as well as the WARNING

  Update: #10698 was merged, leaving:
  ```
  * Checking consistency between dispatch tables and vRPCConvertParams
  ERROR: getblock argument ['verbosity', 'verbose'] has conflicts in vRPCConvertParams conversion specifier [True, False]
  WARNING: conversion mismatch for argument named verbose ([('getblock', False), ('getblockheader', True), ('getmempoolancestors', True), ('getmempooldescendants', True), ('getrawmempool', True), ('getrawtransaction', True)])
  ```

Tree-SHA512: feabebfbeda5d4613b2b9d5265aa6bde4e1a0235297ffd48fa415ad7edc531d9ed7913fe76d191ac60d481a915a326f216bc93de3c671e45e1d14e97d07dea7a
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 19, 2019
c6ec435 [tests] Add bitcoin_cli.py test script (John Newbery)
b23549f [tests] add TestNodeCLI class for calling bitcoin-cli for a node (John Newbery)

Pull request description:

  We don't test bitcoin-cli at all. That means that we can miss inconsistencies between the bitcoin-cli client and the RPC interface, such as bitcoin#10698 and bitcoin#10747. It also means that the various bitcoin-cli options and features are untested and regressions could be silently introduced.

  Let's fix that.

  This PR adds bitcoin-cli testing in the python functional test_framework:

  1. Add a bitcoin_cli.py test script that tests bitcoin-cli. At the moment it only tests that the result of `getinfo` is the same if you run it as an RPC or through bitcoin-cli, but can easily be extended to test additional bitcoin-cli features

  **EDIT: `--usecli` option is moved to a separate PR. This PR now only covers the bitcoin_cli.py test.**

  2. ~Add a `--usecli` option to the test framework. This changes the test to use bitcoin-cli for all RPC calls instead of using direct HTTP requests. This is somewhat experimental. It works for most tests, but there are some cases where it can't work transparently because:~
  - ~the testcase is asserting on a specific error code, and bitcoin-cli returns a different error code from the direct RPC~
  - ~we're sending a very large RPC request (eg `submitblock`) and it can't be serialized into a shell bitcoin-cli call.~

  ~I think that even though `--usecli` doesn't work on all tests, it's still a useful experimental feature. Future potential enhancements:~
  - ~enhance the framework to automatically skip tests that are known to fail with bitcoin-cli if the `--usecli` option is used.~
  - ~run a subset of tests in Travis with `-usecli`~

  This builds on and requires the `TestNode` PR bitcoin#10711 . As an aside, this is a good demonstration of how tidy it is to add additional features/interfaces now that test node logic/state is encapsulated in a TestNode class.

  Addresses bitcoin#10791

Tree-SHA512: a1e6be12e8e007f6f67b3d3bbcd142d835787300831eb38e6027a1ad25ca9d79c4bc99a41b19e31ee95205cba1b3b2d21a688b5909316aad70bfc2b4eb6d8a52
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 23, 2019
c6ec435 [tests] Add bitcoin_cli.py test script (John Newbery)
b23549f [tests] add TestNodeCLI class for calling bitcoin-cli for a node (John Newbery)

Pull request description:

  We don't test bitcoin-cli at all. That means that we can miss inconsistencies between the bitcoin-cli client and the RPC interface, such as bitcoin#10698 and bitcoin#10747. It also means that the various bitcoin-cli options and features are untested and regressions could be silently introduced.

  Let's fix that.

  This PR adds bitcoin-cli testing in the python functional test_framework:

  1. Add a bitcoin_cli.py test script that tests bitcoin-cli. At the moment it only tests that the result of `getinfo` is the same if you run it as an RPC or through bitcoin-cli, but can easily be extended to test additional bitcoin-cli features

  **EDIT: `--usecli` option is moved to a separate PR. This PR now only covers the bitcoin_cli.py test.**

  2. ~Add a `--usecli` option to the test framework. This changes the test to use bitcoin-cli for all RPC calls instead of using direct HTTP requests. This is somewhat experimental. It works for most tests, but there are some cases where it can't work transparently because:~
  - ~the testcase is asserting on a specific error code, and bitcoin-cli returns a different error code from the direct RPC~
  - ~we're sending a very large RPC request (eg `submitblock`) and it can't be serialized into a shell bitcoin-cli call.~

  ~I think that even though `--usecli` doesn't work on all tests, it's still a useful experimental feature. Future potential enhancements:~
  - ~enhance the framework to automatically skip tests that are known to fail with bitcoin-cli if the `--usecli` option is used.~
  - ~run a subset of tests in Travis with `-usecli`~

  This builds on and requires the `TestNode` PR bitcoin#10711 . As an aside, this is a good demonstration of how tidy it is to add additional features/interfaces now that test node logic/state is encapsulated in a TestNode class.

  Addresses bitcoin#10791

Tree-SHA512: a1e6be12e8e007f6f67b3d3bbcd142d835787300831eb38e6027a1ad25ca9d79c4bc99a41b19e31ee95205cba1b3b2d21a688b5909316aad70bfc2b4eb6d8a52
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 24, 2019
*DASH* DOES NOT IMPLEMENT RUNNING THIS IN TRAVIS

77aa9e5 test: Check RPC argument mapping (Wladimir J. van der Laan)

Pull request description:

  Parse the dispatch tables from the server implementation files, and the conversion table from the client (see bitcoin#10751).

  Perform the following consistency checks:

  - Arguments defined in conversion table, must be present in dispatch table. If not, it was probably forgotten to add them to the dispatch table, and they will not work.

  - Arguments defined in conversion table must have the same names as in the dispatch table. If not, they will not work.

  - All aliases for an argument must either be present in the conversion table, or not. Anything in between means an oversight and some aliases won't work.

  Any of these results in an error.

  It also performs a consistency check to see if the same named argument is sometimes converted, and sometimes not. E.g. one RPC call might have a 'verbose' argument that is converted,
  another RPC call might have one that is not converted. This is not necessarily wrong, but points at a possible error (as well as makes the API harder to memorize) - so it is emitted as a warning (could upgrade this to error).

  This test is added to travis and run when `CHECK_DOC`. Currently fails with the following output:
  ```
  * Checking consistency between dispatch tables and vRPCConvertParams
  ERROR: createrawtransaction argument 3 (named optintorbf in vRPCConvertParams) is not defined in dispatch table
  ERROR: getblock argument ['verbosity', 'verbose'] has conflicts in vRPCConvertParams conversion specifier [True, False]
  WARNING: conversion mismatch for argument named verbose ([('getblock', False), ('getblockheader', True), ('getmempoolancestors', True), ('getmempooldescendants', True), ('getrawmempool', True), ('getrawtransaction', True)])
  ```
  - ~bitcoin#10698 fixes the first ERROR~
  - bitcoin#10747 fixes the second ERROR, as well as the WARNING

  Update: bitcoin#10698 was merged, leaving:
  ```
  * Checking consistency between dispatch tables and vRPCConvertParams
  ERROR: getblock argument ['verbosity', 'verbose'] has conflicts in vRPCConvertParams conversion specifier [True, False]
  WARNING: conversion mismatch for argument named verbose ([('getblock', False), ('getblockheader', True), ('getmempoolancestors', True), ('getmempooldescendants', True), ('getrawmempool', True), ('getrawtransaction', True)])
  ```

Tree-SHA512: feabebfbeda5d4613b2b9d5265aa6bde4e1a0235297ffd48fa415ad7edc531d9ed7913fe76d191ac60d481a915a326f216bc93de3c671e45e1d14e97d07dea7a
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 24, 2019
c6ec435 [tests] Add bitcoin_cli.py test script (John Newbery)
b23549f [tests] add TestNodeCLI class for calling bitcoin-cli for a node (John Newbery)

Pull request description:

  We don't test bitcoin-cli at all. That means that we can miss inconsistencies between the bitcoin-cli client and the RPC interface, such as bitcoin#10698 and bitcoin#10747. It also means that the various bitcoin-cli options and features are untested and regressions could be silently introduced.

  Let's fix that.

  This PR adds bitcoin-cli testing in the python functional test_framework:

  1. Add a bitcoin_cli.py test script that tests bitcoin-cli. At the moment it only tests that the result of `getinfo` is the same if you run it as an RPC or through bitcoin-cli, but can easily be extended to test additional bitcoin-cli features

  **EDIT: `--usecli` option is moved to a separate PR. This PR now only covers the bitcoin_cli.py test.**

  2. ~Add a `--usecli` option to the test framework. This changes the test to use bitcoin-cli for all RPC calls instead of using direct HTTP requests. This is somewhat experimental. It works for most tests, but there are some cases where it can't work transparently because:~
  - ~the testcase is asserting on a specific error code, and bitcoin-cli returns a different error code from the direct RPC~
  - ~we're sending a very large RPC request (eg `submitblock`) and it can't be serialized into a shell bitcoin-cli call.~

  ~I think that even though `--usecli` doesn't work on all tests, it's still a useful experimental feature. Future potential enhancements:~
  - ~enhance the framework to automatically skip tests that are known to fail with bitcoin-cli if the `--usecli` option is used.~
  - ~run a subset of tests in Travis with `-usecli`~

  This builds on and requires the `TestNode` PR bitcoin#10711 . As an aside, this is a good demonstration of how tidy it is to add additional features/interfaces now that test node logic/state is encapsulated in a TestNode class.

  Addresses bitcoin#10791

Tree-SHA512: a1e6be12e8e007f6f67b3d3bbcd142d835787300831eb38e6027a1ad25ca9d79c4bc99a41b19e31ee95205cba1b3b2d21a688b5909316aad70bfc2b4eb6d8a52
codablock pushed a commit to codablock/dash that referenced this pull request Sep 24, 2019
c6ec435 [tests] Add bitcoin_cli.py test script (John Newbery)
b23549f [tests] add TestNodeCLI class for calling bitcoin-cli for a node (John Newbery)

Pull request description:

  We don't test bitcoin-cli at all. That means that we can miss inconsistencies between the bitcoin-cli client and the RPC interface, such as bitcoin#10698 and bitcoin#10747. It also means that the various bitcoin-cli options and features are untested and regressions could be silently introduced.

  Let's fix that.

  This PR adds bitcoin-cli testing in the python functional test_framework:

  1. Add a bitcoin_cli.py test script that tests bitcoin-cli. At the moment it only tests that the result of `getinfo` is the same if you run it as an RPC or through bitcoin-cli, but can easily be extended to test additional bitcoin-cli features

  **EDIT: `--usecli` option is moved to a separate PR. This PR now only covers the bitcoin_cli.py test.**

  2. ~Add a `--usecli` option to the test framework. This changes the test to use bitcoin-cli for all RPC calls instead of using direct HTTP requests. This is somewhat experimental. It works for most tests, but there are some cases where it can't work transparently because:~
  - ~the testcase is asserting on a specific error code, and bitcoin-cli returns a different error code from the direct RPC~
  - ~we're sending a very large RPC request (eg `submitblock`) and it can't be serialized into a shell bitcoin-cli call.~

  ~I think that even though `--usecli` doesn't work on all tests, it's still a useful experimental feature. Future potential enhancements:~
  - ~enhance the framework to automatically skip tests that are known to fail with bitcoin-cli if the `--usecli` option is used.~
  - ~run a subset of tests in Travis with `-usecli`~

  This builds on and requires the `TestNode` PR bitcoin#10711 . As an aside, this is a good demonstration of how tidy it is to add additional features/interfaces now that test node logic/state is encapsulated in a TestNode class.

  Addresses bitcoin#10791

Tree-SHA512: a1e6be12e8e007f6f67b3d3bbcd142d835787300831eb38e6027a1ad25ca9d79c4bc99a41b19e31ee95205cba1b3b2d21a688b5909316aad70bfc2b4eb6d8a52
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Dec 21, 2019
*DASH* DOES NOT IMPLEMENT RUNNING THIS IN TRAVIS

77aa9e5 test: Check RPC argument mapping (Wladimir J. van der Laan)

Pull request description:

  Parse the dispatch tables from the server implementation files, and the conversion table from the client (see bitcoin#10751).

  Perform the following consistency checks:

  - Arguments defined in conversion table, must be present in dispatch table. If not, it was probably forgotten to add them to the dispatch table, and they will not work.

  - Arguments defined in conversion table must have the same names as in the dispatch table. If not, they will not work.

  - All aliases for an argument must either be present in the conversion table, or not. Anything in between means an oversight and some aliases won't work.

  Any of these results in an error.

  It also performs a consistency check to see if the same named argument is sometimes converted, and sometimes not. E.g. one RPC call might have a 'verbose' argument that is converted,
  another RPC call might have one that is not converted. This is not necessarily wrong, but points at a possible error (as well as makes the API harder to memorize) - so it is emitted as a warning (could upgrade this to error).

  This test is added to travis and run when `CHECK_DOC`. Currently fails with the following output:
  ```
  * Checking consistency between dispatch tables and vRPCConvertParams
  ERROR: createrawtransaction argument 3 (named optintorbf in vRPCConvertParams) is not defined in dispatch table
  ERROR: getblock argument ['verbosity', 'verbose'] has conflicts in vRPCConvertParams conversion specifier [True, False]
  WARNING: conversion mismatch for argument named verbose ([('getblock', False), ('getblockheader', True), ('getmempoolancestors', True), ('getmempooldescendants', True), ('getrawmempool', True), ('getrawtransaction', True)])
  ```
  - ~bitcoin#10698 fixes the first ERROR~
  - bitcoin#10747 fixes the second ERROR, as well as the WARNING

  Update: bitcoin#10698 was merged, leaving:
  ```
  * Checking consistency between dispatch tables and vRPCConvertParams
  ERROR: getblock argument ['verbosity', 'verbose'] has conflicts in vRPCConvertParams conversion specifier [True, False]
  WARNING: conversion mismatch for argument named verbose ([('getblock', False), ('getblockheader', True), ('getmempoolancestors', True), ('getmempooldescendants', True), ('getrawmempool', True), ('getrawtransaction', True)])
  ```

Tree-SHA512: feabebfbeda5d4613b2b9d5265aa6bde4e1a0235297ffd48fa415ad7edc531d9ed7913fe76d191ac60d481a915a326f216bc93de3c671e45e1d14e97d07dea7a
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 2, 2020
*DASH* DOES NOT IMPLEMENT RUNNING THIS IN TRAVIS

77aa9e5 test: Check RPC argument mapping (Wladimir J. van der Laan)

Pull request description:

  Parse the dispatch tables from the server implementation files, and the conversion table from the client (see bitcoin#10751).

  Perform the following consistency checks:

  - Arguments defined in conversion table, must be present in dispatch table. If not, it was probably forgotten to add them to the dispatch table, and they will not work.

  - Arguments defined in conversion table must have the same names as in the dispatch table. If not, they will not work.

  - All aliases for an argument must either be present in the conversion table, or not. Anything in between means an oversight and some aliases won't work.

  Any of these results in an error.

  It also performs a consistency check to see if the same named argument is sometimes converted, and sometimes not. E.g. one RPC call might have a 'verbose' argument that is converted,
  another RPC call might have one that is not converted. This is not necessarily wrong, but points at a possible error (as well as makes the API harder to memorize) - so it is emitted as a warning (could upgrade this to error).

  This test is added to travis and run when `CHECK_DOC`. Currently fails with the following output:
  ```
  * Checking consistency between dispatch tables and vRPCConvertParams
  ERROR: createrawtransaction argument 3 (named optintorbf in vRPCConvertParams) is not defined in dispatch table
  ERROR: getblock argument ['verbosity', 'verbose'] has conflicts in vRPCConvertParams conversion specifier [True, False]
  WARNING: conversion mismatch for argument named verbose ([('getblock', False), ('getblockheader', True), ('getmempoolancestors', True), ('getmempooldescendants', True), ('getrawmempool', True), ('getrawtransaction', True)])
  ```
  - ~bitcoin#10698 fixes the first ERROR~
  - bitcoin#10747 fixes the second ERROR, as well as the WARNING

  Update: bitcoin#10698 was merged, leaving:
  ```
  * Checking consistency between dispatch tables and vRPCConvertParams
  ERROR: getblock argument ['verbosity', 'verbose'] has conflicts in vRPCConvertParams conversion specifier [True, False]
  WARNING: conversion mismatch for argument named verbose ([('getblock', False), ('getblockheader', True), ('getmempoolancestors', True), ('getmempooldescendants', True), ('getrawmempool', True), ('getrawtransaction', True)])
  ```

Tree-SHA512: feabebfbeda5d4613b2b9d5265aa6bde4e1a0235297ffd48fa415ad7edc531d9ed7913fe76d191ac60d481a915a326f216bc93de3c671e45e1d14e97d07dea7a
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 4, 2020
*DASH* DOES NOT IMPLEMENT RUNNING THIS IN TRAVIS

77aa9e5 test: Check RPC argument mapping (Wladimir J. van der Laan)

Pull request description:

  Parse the dispatch tables from the server implementation files, and the conversion table from the client (see bitcoin#10751).

  Perform the following consistency checks:

  - Arguments defined in conversion table, must be present in dispatch table. If not, it was probably forgotten to add them to the dispatch table, and they will not work.

  - Arguments defined in conversion table must have the same names as in the dispatch table. If not, they will not work.

  - All aliases for an argument must either be present in the conversion table, or not. Anything in between means an oversight and some aliases won't work.

  Any of these results in an error.

  It also performs a consistency check to see if the same named argument is sometimes converted, and sometimes not. E.g. one RPC call might have a 'verbose' argument that is converted,
  another RPC call might have one that is not converted. This is not necessarily wrong, but points at a possible error (as well as makes the API harder to memorize) - so it is emitted as a warning (could upgrade this to error).

  This test is added to travis and run when `CHECK_DOC`. Currently fails with the following output:
  ```
  * Checking consistency between dispatch tables and vRPCConvertParams
  ERROR: createrawtransaction argument 3 (named optintorbf in vRPCConvertParams) is not defined in dispatch table
  ERROR: getblock argument ['verbosity', 'verbose'] has conflicts in vRPCConvertParams conversion specifier [True, False]
  WARNING: conversion mismatch for argument named verbose ([('getblock', False), ('getblockheader', True), ('getmempoolancestors', True), ('getmempooldescendants', True), ('getrawmempool', True), ('getrawtransaction', True)])
  ```
  - ~bitcoin#10698 fixes the first ERROR~
  - bitcoin#10747 fixes the second ERROR, as well as the WARNING

  Update: bitcoin#10698 was merged, leaving:
  ```
  * Checking consistency between dispatch tables and vRPCConvertParams
  ERROR: getblock argument ['verbosity', 'verbose'] has conflicts in vRPCConvertParams conversion specifier [True, False]
  WARNING: conversion mismatch for argument named verbose ([('getblock', False), ('getblockheader', True), ('getmempoolancestors', True), ('getmempooldescendants', True), ('getrawmempool', True), ('getrawtransaction', True)])
  ```

Tree-SHA512: feabebfbeda5d4613b2b9d5265aa6bde4e1a0235297ffd48fa415ad7edc531d9ed7913fe76d191ac60d481a915a326f216bc93de3c671e45e1d14e97d07dea7a
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 4, 2020
*DASH* DOES NOT IMPLEMENT RUNNING THIS IN TRAVIS

77aa9e5 test: Check RPC argument mapping (Wladimir J. van der Laan)

Pull request description:

  Parse the dispatch tables from the server implementation files, and the conversion table from the client (see bitcoin#10751).

  Perform the following consistency checks:

  - Arguments defined in conversion table, must be present in dispatch table. If not, it was probably forgotten to add them to the dispatch table, and they will not work.

  - Arguments defined in conversion table must have the same names as in the dispatch table. If not, they will not work.

  - All aliases for an argument must either be present in the conversion table, or not. Anything in between means an oversight and some aliases won't work.

  Any of these results in an error.

  It also performs a consistency check to see if the same named argument is sometimes converted, and sometimes not. E.g. one RPC call might have a 'verbose' argument that is converted,
  another RPC call might have one that is not converted. This is not necessarily wrong, but points at a possible error (as well as makes the API harder to memorize) - so it is emitted as a warning (could upgrade this to error).

  This test is added to travis and run when `CHECK_DOC`. Currently fails with the following output:
  ```
  * Checking consistency between dispatch tables and vRPCConvertParams
  ERROR: createrawtransaction argument 3 (named optintorbf in vRPCConvertParams) is not defined in dispatch table
  ERROR: getblock argument ['verbosity', 'verbose'] has conflicts in vRPCConvertParams conversion specifier [True, False]
  WARNING: conversion mismatch for argument named verbose ([('getblock', False), ('getblockheader', True), ('getmempoolancestors', True), ('getmempooldescendants', True), ('getrawmempool', True), ('getrawtransaction', True)])
  ```
  - ~bitcoin#10698 fixes the first ERROR~
  - bitcoin#10747 fixes the second ERROR, as well as the WARNING

  Update: bitcoin#10698 was merged, leaving:
  ```
  * Checking consistency between dispatch tables and vRPCConvertParams
  ERROR: getblock argument ['verbosity', 'verbose'] has conflicts in vRPCConvertParams conversion specifier [True, False]
  WARNING: conversion mismatch for argument named verbose ([('getblock', False), ('getblockheader', True), ('getmempoolancestors', True), ('getmempooldescendants', True), ('getrawmempool', True), ('getrawtransaction', True)])
  ```

Tree-SHA512: feabebfbeda5d4613b2b9d5265aa6bde4e1a0235297ffd48fa415ad7edc531d9ed7913fe76d191ac60d481a915a326f216bc93de3c671e45e1d14e97d07dea7a
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 10, 2020
*DASH* DOES NOT IMPLEMENT RUNNING THIS IN TRAVIS

77aa9e5 test: Check RPC argument mapping (Wladimir J. van der Laan)

Pull request description:

  Parse the dispatch tables from the server implementation files, and the conversion table from the client (see bitcoin#10751).

  Perform the following consistency checks:

  - Arguments defined in conversion table, must be present in dispatch table. If not, it was probably forgotten to add them to the dispatch table, and they will not work.

  - Arguments defined in conversion table must have the same names as in the dispatch table. If not, they will not work.

  - All aliases for an argument must either be present in the conversion table, or not. Anything in between means an oversight and some aliases won't work.

  Any of these results in an error.

  It also performs a consistency check to see if the same named argument is sometimes converted, and sometimes not. E.g. one RPC call might have a 'verbose' argument that is converted,
  another RPC call might have one that is not converted. This is not necessarily wrong, but points at a possible error (as well as makes the API harder to memorize) - so it is emitted as a warning (could upgrade this to error).

  This test is added to travis and run when `CHECK_DOC`. Currently fails with the following output:
  ```
  * Checking consistency between dispatch tables and vRPCConvertParams
  ERROR: createrawtransaction argument 3 (named optintorbf in vRPCConvertParams) is not defined in dispatch table
  ERROR: getblock argument ['verbosity', 'verbose'] has conflicts in vRPCConvertParams conversion specifier [True, False]
  WARNING: conversion mismatch for argument named verbose ([('getblock', False), ('getblockheader', True), ('getmempoolancestors', True), ('getmempooldescendants', True), ('getrawmempool', True), ('getrawtransaction', True)])
  ```
  - ~bitcoin#10698 fixes the first ERROR~
  - bitcoin#10747 fixes the second ERROR, as well as the WARNING

  Update: bitcoin#10698 was merged, leaving:
  ```
  * Checking consistency between dispatch tables and vRPCConvertParams
  ERROR: getblock argument ['verbosity', 'verbose'] has conflicts in vRPCConvertParams conversion specifier [True, False]
  WARNING: conversion mismatch for argument named verbose ([('getblock', False), ('getblockheader', True), ('getmempoolancestors', True), ('getmempooldescendants', True), ('getrawmempool', True), ('getrawtransaction', True)])
  ```

Tree-SHA512: feabebfbeda5d4613b2b9d5265aa6bde4e1a0235297ffd48fa415ad7edc531d9ed7913fe76d191ac60d481a915a326f216bc93de3c671e45e1d14e97d07dea7a
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 10, 2020
*DASH* DOES NOT IMPLEMENT RUNNING THIS IN TRAVIS

77aa9e5 test: Check RPC argument mapping (Wladimir J. van der Laan)

Pull request description:

  Parse the dispatch tables from the server implementation files, and the conversion table from the client (see bitcoin#10751).

  Perform the following consistency checks:

  - Arguments defined in conversion table, must be present in dispatch table. If not, it was probably forgotten to add them to the dispatch table, and they will not work.

  - Arguments defined in conversion table must have the same names as in the dispatch table. If not, they will not work.

  - All aliases for an argument must either be present in the conversion table, or not. Anything in between means an oversight and some aliases won't work.

  Any of these results in an error.

  It also performs a consistency check to see if the same named argument is sometimes converted, and sometimes not. E.g. one RPC call might have a 'verbose' argument that is converted,
  another RPC call might have one that is not converted. This is not necessarily wrong, but points at a possible error (as well as makes the API harder to memorize) - so it is emitted as a warning (could upgrade this to error).

  This test is added to travis and run when `CHECK_DOC`. Currently fails with the following output:
  ```
  * Checking consistency between dispatch tables and vRPCConvertParams
  ERROR: createrawtransaction argument 3 (named optintorbf in vRPCConvertParams) is not defined in dispatch table
  ERROR: getblock argument ['verbosity', 'verbose'] has conflicts in vRPCConvertParams conversion specifier [True, False]
  WARNING: conversion mismatch for argument named verbose ([('getblock', False), ('getblockheader', True), ('getmempoolancestors', True), ('getmempooldescendants', True), ('getrawmempool', True), ('getrawtransaction', True)])
  ```
  - ~bitcoin#10698 fixes the first ERROR~
  - bitcoin#10747 fixes the second ERROR, as well as the WARNING

  Update: bitcoin#10698 was merged, leaving:
  ```
  * Checking consistency between dispatch tables and vRPCConvertParams
  ERROR: getblock argument ['verbosity', 'verbose'] has conflicts in vRPCConvertParams conversion specifier [True, False]
  WARNING: conversion mismatch for argument named verbose ([('getblock', False), ('getblockheader', True), ('getmempoolancestors', True), ('getmempooldescendants', True), ('getrawmempool', True), ('getrawtransaction', True)])
  ```

Tree-SHA512: feabebfbeda5d4613b2b9d5265aa6bde4e1a0235297ffd48fa415ad7edc531d9ed7913fe76d191ac60d481a915a326f216bc93de3c671e45e1d14e97d07dea7a
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 10, 2020
*DASH* DOES NOT IMPLEMENT RUNNING THIS IN TRAVIS

77aa9e5 test: Check RPC argument mapping (Wladimir J. van der Laan)

Pull request description:

  Parse the dispatch tables from the server implementation files, and the conversion table from the client (see bitcoin#10751).

  Perform the following consistency checks:

  - Arguments defined in conversion table, must be present in dispatch table. If not, it was probably forgotten to add them to the dispatch table, and they will not work.

  - Arguments defined in conversion table must have the same names as in the dispatch table. If not, they will not work.

  - All aliases for an argument must either be present in the conversion table, or not. Anything in between means an oversight and some aliases won't work.

  Any of these results in an error.

  It also performs a consistency check to see if the same named argument is sometimes converted, and sometimes not. E.g. one RPC call might have a 'verbose' argument that is converted,
  another RPC call might have one that is not converted. This is not necessarily wrong, but points at a possible error (as well as makes the API harder to memorize) - so it is emitted as a warning (could upgrade this to error).

  This test is added to travis and run when `CHECK_DOC`. Currently fails with the following output:
  ```
  * Checking consistency between dispatch tables and vRPCConvertParams
  ERROR: createrawtransaction argument 3 (named optintorbf in vRPCConvertParams) is not defined in dispatch table
  ERROR: getblock argument ['verbosity', 'verbose'] has conflicts in vRPCConvertParams conversion specifier [True, False]
  WARNING: conversion mismatch for argument named verbose ([('getblock', False), ('getblockheader', True), ('getmempoolancestors', True), ('getmempooldescendants', True), ('getrawmempool', True), ('getrawtransaction', True)])
  ```
  - ~bitcoin#10698 fixes the first ERROR~
  - bitcoin#10747 fixes the second ERROR, as well as the WARNING

  Update: bitcoin#10698 was merged, leaving:
  ```
  * Checking consistency between dispatch tables and vRPCConvertParams
  ERROR: getblock argument ['verbosity', 'verbose'] has conflicts in vRPCConvertParams conversion specifier [True, False]
  WARNING: conversion mismatch for argument named verbose ([('getblock', False), ('getblockheader', True), ('getmempoolancestors', True), ('getmempooldescendants', True), ('getrawmempool', True), ('getrawtransaction', True)])
  ```

Tree-SHA512: feabebfbeda5d4613b2b9d5265aa6bde4e1a0235297ffd48fa415ad7edc531d9ed7913fe76d191ac60d481a915a326f216bc93de3c671e45e1d14e97d07dea7a
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 12, 2020
*DASH* DOES NOT IMPLEMENT RUNNING THIS IN TRAVIS

77aa9e5 test: Check RPC argument mapping (Wladimir J. van der Laan)

Pull request description:

  Parse the dispatch tables from the server implementation files, and the conversion table from the client (see bitcoin#10751).

  Perform the following consistency checks:

  - Arguments defined in conversion table, must be present in dispatch table. If not, it was probably forgotten to add them to the dispatch table, and they will not work.

  - Arguments defined in conversion table must have the same names as in the dispatch table. If not, they will not work.

  - All aliases for an argument must either be present in the conversion table, or not. Anything in between means an oversight and some aliases won't work.

  Any of these results in an error.

  It also performs a consistency check to see if the same named argument is sometimes converted, and sometimes not. E.g. one RPC call might have a 'verbose' argument that is converted,
  another RPC call might have one that is not converted. This is not necessarily wrong, but points at a possible error (as well as makes the API harder to memorize) - so it is emitted as a warning (could upgrade this to error).

  This test is added to travis and run when `CHECK_DOC`. Currently fails with the following output:
  ```
  * Checking consistency between dispatch tables and vRPCConvertParams
  ERROR: createrawtransaction argument 3 (named optintorbf in vRPCConvertParams) is not defined in dispatch table
  ERROR: getblock argument ['verbosity', 'verbose'] has conflicts in vRPCConvertParams conversion specifier [True, False]
  WARNING: conversion mismatch for argument named verbose ([('getblock', False), ('getblockheader', True), ('getmempoolancestors', True), ('getmempooldescendants', True), ('getrawmempool', True), ('getrawtransaction', True)])
  ```
  - ~bitcoin#10698 fixes the first ERROR~
  - bitcoin#10747 fixes the second ERROR, as well as the WARNING

  Update: bitcoin#10698 was merged, leaving:
  ```
  * Checking consistency between dispatch tables and vRPCConvertParams
  ERROR: getblock argument ['verbosity', 'verbose'] has conflicts in vRPCConvertParams conversion specifier [True, False]
  WARNING: conversion mismatch for argument named verbose ([('getblock', False), ('getblockheader', True), ('getmempoolancestors', True), ('getmempooldescendants', True), ('getrawmempool', True), ('getrawtransaction', True)])
  ```

Tree-SHA512: feabebfbeda5d4613b2b9d5265aa6bde4e1a0235297ffd48fa415ad7edc531d9ed7913fe76d191ac60d481a915a326f216bc93de3c671e45e1d14e97d07dea7a
barrystyle pushed a commit to PACGlobalOfficial/PAC that referenced this pull request Jan 22, 2020
c6ec435 [tests] Add bitcoin_cli.py test script (John Newbery)
b23549f [tests] add TestNodeCLI class for calling bitcoin-cli for a node (John Newbery)

Pull request description:

  We don't test bitcoin-cli at all. That means that we can miss inconsistencies between the bitcoin-cli client and the RPC interface, such as bitcoin#10698 and bitcoin#10747. It also means that the various bitcoin-cli options and features are untested and regressions could be silently introduced.

  Let's fix that.

  This PR adds bitcoin-cli testing in the python functional test_framework:

  1. Add a bitcoin_cli.py test script that tests bitcoin-cli. At the moment it only tests that the result of `getinfo` is the same if you run it as an RPC or through bitcoin-cli, but can easily be extended to test additional bitcoin-cli features

  **EDIT: `--usecli` option is moved to a separate PR. This PR now only covers the bitcoin_cli.py test.**

  2. ~Add a `--usecli` option to the test framework. This changes the test to use bitcoin-cli for all RPC calls instead of using direct HTTP requests. This is somewhat experimental. It works for most tests, but there are some cases where it can't work transparently because:~
  - ~the testcase is asserting on a specific error code, and bitcoin-cli returns a different error code from the direct RPC~
  - ~we're sending a very large RPC request (eg `submitblock`) and it can't be serialized into a shell bitcoin-cli call.~

  ~I think that even though `--usecli` doesn't work on all tests, it's still a useful experimental feature. Future potential enhancements:~
  - ~enhance the framework to automatically skip tests that are known to fail with bitcoin-cli if the `--usecli` option is used.~
  - ~run a subset of tests in Travis with `-usecli`~

  This builds on and requires the `TestNode` PR bitcoin#10711 . As an aside, this is a good demonstration of how tidy it is to add additional features/interfaces now that test node logic/state is encapsulated in a TestNode class.

  Addresses bitcoin#10791

Tree-SHA512: a1e6be12e8e007f6f67b3d3bbcd142d835787300831eb38e6027a1ad25ca9d79c4bc99a41b19e31ee95205cba1b3b2d21a688b5909316aad70bfc2b4eb6d8a52
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants