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

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 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

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

andvgal referenced this in commit dddc4d0037 on Jan 6, 2019

MarcoFalke locked this on Sep 8, 2021