Skip to content

Conversation

@kallewoof
Copy link
Contributor

This PR adjusts the two issues I encountered while developing a tool that converts RPCHelpMan objects into bindings for other language(s).

The first is in createrawtransaction, where the address part, e.g. bc1qabc in

createrawtransaction '[]' '[{"bc1qabc": 1.0}]'

is declared as a Type::OBJ, when in reality it should be a Type::OBJ_USER_KEYS, defined as such:

OBJ_USER_KEYS, //!< Special type where the user must set the keys e.g. to define multiple addresses; as opposed to e.g. an options object where the keys are predefined

(coincidentally, this is the first and only (afaict) usage of this RPCArg::Type).

The second is in the listaddressgroupings RPC, which returns an array of arrays of arrays, where the innermost one is a tuple-thingie with an optional 3rd item; this is an ARR_FIXED, not an ARR.

kallewoof added 3 commits May 9, 2021 22:22
The OBJ type is for actual objects with defined keys; OBJ_USER_KEYS is for objects with user-defined keys (such as the bitcoin address(es) in the createrawtransaction output object.
ARR_FIXED is for cases like this, where the elements are in an array for convenience, rather than due to being dynamically sized lists.
@maflcko
Copy link
Member

maflcko commented May 10, 2021

Nice find. Rendered diff:

diff --git a/createrawtransaction b/createrawtransaction
index b488df3..cd68d55 100644
--- a/createrawtransaction
+++ b/createrawtransaction
@@ -1,4 +1,4 @@
-createrawtransaction [{"txid":"hex","vout":n,"sequence":n},...] [{"address":amount},{"data":"hex"},...] ( locktime replaceable )
+createrawtransaction [{"txid":"hex","vout":n,"sequence":n},...] [{"address":amount,...},{"data":"hex"},...] ( locktime replaceable )
 
 Create a transaction spending the given inputs and creating new outputs.
 Outputs can be addresses or data.
@@ -23,6 +23,7 @@ Arguments:
      [
        {                       (json object)
          "address": amount,    (numeric or string, required) A key-value pair. The key (string) is the bitcoin address, the value (float or string) is the amount in BTC
+         ...
        },
        {                       (json object)
          "data": "hex",        (string, required) A key-value pair. The key must be "data", the value is hex-encoded data
diff --git a/listaddressgroupings b/listaddressgroupings
index 860fe09..552faaa 100644
--- a/listaddressgroupings
+++ b/listaddressgroupings
@@ -10,8 +10,7 @@ Result:
     [           (json array)
       "str",    (string) The bitcoin address
       n,        (numeric) The amount in BTC
-      "str",    (string, optional) The label
-      ...
+      "str"     (string, optional) The label
     ],
     ...
   ],

@maflcko
Copy link
Member

maflcko commented May 10, 2021

ACK 7031721 🐀

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 7031721f2cc3eef30c46ff50c52328e9ba8090e0 🐀
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUharwv9Hh5fzDU3kxyvsQzhU8YcNU+wBVd4QW8Abe4VoJQIyNqQPRC4z7VCHVV1
/3Hl8lHVpc7IwqrhFFMGWBSpzYN5WUPgxsv9uarGhS9OtzcwQQDi5m/u+XoDQLf7
gzWk3didSwZ00nL5hMheC4WG3FYhSOPEXR9dqB4hRllVn6aUPibKf5Qq2wlsb1A8
b9aM2tNVl1FVlUyraNLt7L1ECmedvJq/X8g1nZjZ0IwX9fQ6g/VmCMDzXI9WFDGB
XH6FKFicK6ckKupzQxQeAN7R/PjCrMeQn0ED7JefFCWn0w20sHpDGlRYJ1f73pi5
6YWiccycvctKtMwjK4QAYdq/CHGgwEA/5DySZ8X4LcKHcmdBh2yq5Y+H1lPnWvDj
W1pjjePMolhJF08sFZQDqE7xxdo/zu9VOMJNVlJby3/E/HRQMlP7BrcfuG8Eib8b
FJNL1QI7X3i7KJioPGd23rQbFpaUeGvy2yYFN2PsM2LIumqrUXzU+MIPmgWCmjDT
fjoOz+7C
=CcfS
-----END PGP SIGNATURE-----

Timestamp of file with hash cea78a19ab915d79f1fd04d67ae9ab391a7807d117131861f2c3f94c0f7e29e7 -

@maflcko maflcko merged commit 2a22d90 into bitcoin:master May 10, 2021
@kallewoof kallewoof deleted the 202104-rpctypes branch May 10, 2021 07:00
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 10, 2021
7031721 rpc/listaddressgroupings: redefine inner-most array as ARR_FIXED (Karl-Johan Alm)
8500f7b rpc/createrawtransaction: redefine addresses as OBJ_USER_KEYS (Karl-Johan Alm)
d9e2183 rpc: include OBJ_USER_KEY in RPCArg constructor checks (Karl-Johan Alm)

Pull request description:

  This PR adjusts the two issues I encountered while developing a tool that converts RPCHelpMan objects into bindings for other language(s).

  The first is in createrawtransaction, where the address part, e.g. bc1qabc in

  > createrawtransaction '[]' '[{"bc1qabc": 1.0}]'

  is declared as a `Type::OBJ`, when in reality it should be a `Type::OBJ_USER_KEYS`, defined as such:

  https://github.com/bitcoin/bitcoin/blob/5925f1e652768a9502831b9ccf78d16cf3c37d29/src/rpc/util.h#L126

  (coincidentally, this is the first and only (afaict) usage of this `RPCArg::Type`).

  The second is in the `listaddressgroupings` RPC, which returns an array of arrays of arrays, where the innermost one is a tuple-thingie with an optional 3rd item; this is an `ARR_FIXED`, not an `ARR`.

ACKs for top commit:
  MarcoFalke:
    ACK 7031721 🐀

Tree-SHA512: 769377416c6226d1738a956fb685498e009f9e7eb2d45bc679b81c5364b9520fdbcb49392c937ab45598aa0d33589e8e6a59ccc101cf8d8e7dfdafd58d4eefd0
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request May 20, 2021
6e2eb0d rpc/wallet: use OMITTED_NAMED_ARG instead of Default(VNULL) (Karl-Johan Alm)
4983f4c rpc/createwallet: omitted named arguments (Karl-Johan Alm)
dc4db23 rpc: address:amount dictionaries are OBJ_USER_KEYS (Karl-Johan Alm)
c8cf0a3 rpc/getpeerinfo: bytesrecv_per_msg is a dynamic dictionary (Karl-Johan Alm)
eb4fb7e rpc/gettxoutsetinfo: hash_or_height is a named argument (Karl-Johan Alm)

Pull request description:

  This is a follow-up to bitcoin#21897, and I believe covers the remaining cases, at least that I could find.

  Edited to remove unrelated information about a side project.

ACKs for top commit:
  laanwj:
    Documentation diff ACK 6e2eb0d
  promag:
    Code review ACK 6e2eb0d.

Tree-SHA512: d26f6e074e13d64bbca2a114a0adc7f905d47d238c4e9bc49f70ca0b775afbebf9879fc3794ab29dc316a6dbd00ba8cbeb01197e236ee4ab2e9854db25f23f04
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 20, 2021
6e2eb0d rpc/wallet: use OMITTED_NAMED_ARG instead of Default(VNULL) (Karl-Johan Alm)
4983f4c rpc/createwallet: omitted named arguments (Karl-Johan Alm)
dc4db23 rpc: address:amount dictionaries are OBJ_USER_KEYS (Karl-Johan Alm)
c8cf0a3 rpc/getpeerinfo: bytesrecv_per_msg is a dynamic dictionary (Karl-Johan Alm)
eb4fb7e rpc/gettxoutsetinfo: hash_or_height is a named argument (Karl-Johan Alm)

Pull request description:

  This is a follow-up to bitcoin#21897, and I believe covers the remaining cases, at least that I could find.

  Edited to remove unrelated information about a side project.

ACKs for top commit:
  laanwj:
    Documentation diff ACK 6e2eb0d
  promag:
    Code review ACK 6e2eb0d.

Tree-SHA512: d26f6e074e13d64bbca2a114a0adc7f905d47d238c4e9bc49f70ca0b775afbebf9879fc3794ab29dc316a6dbd00ba8cbeb01197e236ee4ab2e9854db25f23f04
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants