[rpc] fundrawtransaction feeRate: Use BTC/kB #8153

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:Mf1606-univalAny changing 5 files +51 −21

MarcoFalke commented at 6:12 PM on June 6, 2016: member

No description provided.
[rpc] fundrawtransaction: Fix help text and interface faf82e8fc8
MarcoFalke added the label RPC/REST/ZMQ on Jun 6, 2016
luke-jr commented at 6:17 PM on June 6, 2016: member

Should actually be BTC (or satoshis?) per byte, since we no longer do it per kB...
sipa commented at 6:18 PM on June 6, 2016: member

All RPC arguments use BTC/kByte, no?
jonasschnelli commented at 6:19 PM on June 6, 2016: contributor

It should be the same format than bitcoind spit out when one calls estimatefee.
MarcoFalke cross-referenced this on Jun 6, 2016 from issue [rpc] fundrawtransaction: Fix help text by MarcoFalke
MarcoFalke commented at 7:51 PM on June 6, 2016: member

@jonasschnelli et al: Added a commit so I don't have to modify the univalue subtree. (Added a UniValueType wrapper instead)

in qa/rpc-tests/fundrawtransaction.py:None in a9a64df3b4 outdated

 680 | @@ -681,9 +681,9 @@ def run_test(self):
 681 |          inputs = []
 682 |          outputs = {self.nodes[2].getnewaddress() : 1}
 683 |          rawtx = self.nodes[3].createrawtransaction(inputs, outputs)
 684 | -        result = self.nodes[3].fundrawtransaction(rawtx, )
 685 | -        result2 = self.nodes[3].fundrawtransaction(rawtx, {"feeRate": 2000})
 686 | -        result3 = self.nodes[3].fundrawtransaction(rawtx, {"feeRate": 10000})
 687 | +        result = self.nodes[3].fundrawtransaction(rawtx) # 1000 sat via settxfee

fanquake commented at 2:27 AM on June 7, 2016:

nit s/sat/set

paveljanik commented at 4:03 AM on June 7, 2016:

it is probably satoshis. But not very clear, yes.

MarcoFalke commented at 7:06 AM on June 7, 2016:

Thanks, will fix the comment.

laanwj commented at 6:46 AM on June 7, 2016: member

BTC/kB is fine, we use that for fee rates on the interface everywhere. The point of this change is to reduce the variety of different ways in which the same is expressed on the interface, not come up with something new.

utACK https://github.com/bitcoin/bitcoin/pull/8153/commits/faf82e8fc819b2f1f8b60983ac72cb111c47e8ba
MarcoFalke force-pushed on Jun 7, 2016
MarcoFalke commented at 7:05 AM on June 7, 2016: member

I think @luke-jr was referring to kB no longer being the smallest unit. Currently we use Byte to be the smallest unit.

in src/rpc/rawtransaction.cpp:None in 7817d3fee1 outdated

 674 | @@ -675,7 +675,12 @@ UniValue signrawtransaction(const UniValue& params, bool fHelp)
 675 |  
 676 |              UniValue prevOut = p.get_obj();
 677 |  
 678 | -            RPCTypeCheckObj(prevOut, boost::assign::map_list_of("txid", UniValue::VSTR)("vout", UniValue::VNUM)("scriptPubKey", UniValue::VSTR));
 679 | +            RPCTypeCheckObj(prevOut,

laanwj commented at 7:14 AM on June 7, 2016:

Looks muc hbetter in c++11 syntax

MarcoFalke force-pushed on Jun 7, 2016
MarcoFalke commented at 7:17 AM on June 7, 2016: member

(Force pushed after fixing comment-nit)
jonasschnelli commented at 9:11 AM on June 7, 2016: contributor

Not sure if I got this right, but why not just removing the "feeRate" from the RPCTypeCheckObj check (it's optional anyways)? I don't see the reason for the new UniValueType type.
MarcoFalke commented at 9:27 AM on June 7, 2016: member

We also want to suppress passing in unrecognized parameters. (Try removing it and see where the rpc tests fail. Hint: fStrict is set to true)
jonasschnelli commented at 9:32 AM on June 7, 2016: contributor

Ah. Right. Makes sense. Is there no way to avoid the UniValueType wrapper? Using UniValue::VNULL for a type-independent check? Or would a C++11 union be simpler?

in src/rpc/server.h:None in fab362944c outdated

  31 | @@ -32,6 +32,15 @@ namespace RPCServer
  32 |  class CBlockIndex;
  33 |  class CNetAddr;
  34 |  
  35 | +/** Wrapper for UniValue::VType, which includes typeAny:
  36 | + * Used to denote don't care type. Only used by RPCTypeCheckObj */
  37 | +typedef struct UniValueType {

sipa commented at 9:38 AM on June 7, 2016:

typedef is not needed for structs and classes in C++.

MarcoFalke commented at 10:11 AM on June 7, 2016:

Fixed.

Also changed to union as suggested by @jonasschnelli

MarcoFalke force-pushed on Jun 7, 2016
jonasschnelli commented at 10:55 AM on June 7, 2016: contributor

Nice! utACK fa51551b7f5c2dbfae01f5cee8ee8251868609e2

[rpc] fundrawtransaction feeRate: Use BTC/kB

Also introduce UniValueType

UniValueType is a wrapper for UniValue::VType which allows setting
a typeAny flag. This flag indicates the type does not matter.

(Used by RPCTypeCheckObj)

fa7f4f577c

in src/rpc/server.cpp:None in fa51551b7f outdated
```
 102 |          if (!fAllowNull && v.isNull())
 103 |              throw JSONRPCError(RPC_TYPE_ERROR, strprintf("Missing %s", t.first));
 104 |  
 105 | -        if (!((v.type() == t.second) || (fAllowNull && (v.isNull()))))
 106 | -        {
 107 | +        if (!(t.second.typeAny || v.type() == t.second.type || (fAllowNull && v.isNull()))) {
```
sipa commented at 11:02 AM on June 7, 2016:

This is not valid. You can only access fields of a union that you know was the last one assigned to. Testing both typeAny and type certains violates that property at least once.

MarcoFalke commented at 11:29 AM on June 7, 2016:

Right, I should probably just do s/union/struct/ to revert to struct.

laanwj commented at 12:00 PM on June 7, 2016:

I agree. It's very easy to violate the requirements of using an union accidentally, resulting in horrible hard to debug issues, I'd prefer not using them unless it's a place where the additional memory usage is critical. I don't think that's the case here.

MarcoFalke commented at 12:26 PM on June 7, 2016:

done. Hope I got it right this time. :+1:
MarcoFalke force-pushed on Jun 7, 2016
in src/wallet/rpcwallet.cpp:None in fa7f4f577c
```
2442 | @@ -2431,7 +2443,7 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp)
2443 |  
2444 |          if (options.exists("feeRate"))
2445 |          {
2446 | -            feeRate = CFeeRate(options["feeRate"].get_real());
```
laanwj commented at 12:22 PM on June 7, 2016:

We slipped up with the review for #7967, shouldn't have allowed using get_real here. Good that this is being replaced.

MarcoFalke commented at 12:32 PM on June 7, 2016:

On the other hand, imagine if someone noticed it while reviewing #7967: The pull probably would have never made it into 0.13 before feature freeze, because adding the anytype is a non-trivial refactor and review is hard to do when there is substantial refactoring going on while adding new features.

Looking at it from this perspective, it makes sense to have two pulls to aid review.

laanwj commented at 1:28 PM on June 7, 2016:

I agree
laanwj merged this on Jun 8, 2016
laanwj closed this on Jun 8, 2016
laanwj referenced this in commit 75ec320a0d on Jun 8, 2016
laanwj commented at 12:15 PM on June 8, 2016: member

utACK fa7f4f5
laanwj cross-referenced this on Jun 8, 2016 from issue TODO for release notes 0.13.0 by laanwj
MarcoFalke deleted the branch on Jun 8, 2016
codablock referenced this in commit f6c85696fb on Sep 16, 2017
codablock referenced this in commit be6d30acb6 on Sep 19, 2017
codablock referenced this in commit 05419aba3c on Dec 22, 2017
dagurval cross-referenced this on Feb 1, 2018 from issue fundrawtransaction cherries by dagurval
andvgal referenced this in commit dddc4d0037 on Jan 6, 2019
bitcoin locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#7678 TODO for release notes 0.13.0 #7967 [RPC] add feerate option to fundrawtransaction #8144 [rpc] fundrawtransaction: Fix help text