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

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

Merged
merged 2 commits into from Jun 8, 2016

Conversation

Projects
None yet
7 participants
@MarcoFalke
Member

MarcoFalke commented Jun 6, 2016

No description provided.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jun 6, 2016

Member

Should actually be BTC (or satoshis?) per byte, since we no longer do it per kB...

Member

luke-jr commented Jun 6, 2016

Should actually be BTC (or satoshis?) per byte, since we no longer do it per kB...

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 6, 2016

Member

All RPC arguments use BTC/kByte, no?

Member

sipa commented Jun 6, 2016

All RPC arguments use BTC/kByte, no?

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 6, 2016

Member

It should be the same format than bitcoind spit out when one calls estimatefee.

Member

jonasschnelli commented Jun 6, 2016

It should be the same format than bitcoind spit out when one calls estimatefee.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jun 6, 2016

Member

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

Member

MarcoFalke commented Jun 6, 2016

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

@fanquake

View changes

Show outdated Hide outdated qa/rpc-tests/fundrawtransaction.py
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 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 faf82e8

Member

laanwj commented Jun 7, 2016

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 faf82e8

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jun 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.

Member

MarcoFalke commented Jun 7, 2016

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

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

This comment has been minimized.

@laanwj

laanwj Jun 7, 2016

Member

Looks muc hbetter in c++11 syntax

@laanwj

laanwj Jun 7, 2016

Member

Looks muc hbetter in c++11 syntax

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jun 7, 2016

Member

(Force pushed after fixing comment-nit)

Member

MarcoFalke commented Jun 7, 2016

(Force pushed after fixing comment-nit)

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 7, 2016

Member

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.

Member

jonasschnelli commented Jun 7, 2016

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

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jun 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)

Member

MarcoFalke commented Jun 7, 2016

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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 7, 2016

Member

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?

Member

jonasschnelli commented Jun 7, 2016

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?

@sipa

View changes

Show outdated Hide outdated src/rpc/server.h
@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 7, 2016

Member

Nice!
utACK fa51551b7f5c2dbfae01f5cee8ee8251868609e2

Member

jonasschnelli commented Jun 7, 2016

Nice!
utACK fa51551b7f5c2dbfae01f5cee8ee8251868609e2

const UniValue& v = find_value(o, t.first);
if (!fAllowNull && v.isNull())
throw JSONRPCError(RPC_TYPE_ERROR, strprintf("Missing %s", t.first));
if (!((v.type() == t.second) || (fAllowNull && (v.isNull()))))
{
if (!(t.second.typeAny || v.type() == t.second.type || (fAllowNull && v.isNull()))) {

This comment has been minimized.

@sipa

sipa Jun 7, 2016

Member

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.

@sipa

sipa Jun 7, 2016

Member

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.

This comment has been minimized.

@MarcoFalke

MarcoFalke Jun 7, 2016

Member

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

@MarcoFalke

MarcoFalke Jun 7, 2016

Member

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

This comment has been minimized.

@laanwj

laanwj Jun 7, 2016

Member

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.

@laanwj

laanwj Jun 7, 2016

Member

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.

This comment has been minimized.

@MarcoFalke

MarcoFalke Jun 7, 2016

Member

done. Hope I got it right this time. 👍

@MarcoFalke

MarcoFalke Jun 7, 2016

Member

done. Hope I got it right this time. 👍

[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)
@@ -2431,7 +2443,7 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp)
if (options.exists("feeRate"))
{
feeRate = CFeeRate(options["feeRate"].get_real());

This comment has been minimized.

@laanwj

laanwj Jun 7, 2016

Member

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

@laanwj

laanwj Jun 7, 2016

Member

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

This comment has been minimized.

@MarcoFalke

MarcoFalke Jun 7, 2016

Member

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.

@MarcoFalke

MarcoFalke Jun 7, 2016

Member

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.

This comment has been minimized.

@laanwj

laanwj Jun 7, 2016

Member

I agree

@laanwj

laanwj Jun 7, 2016

Member

I agree

@laanwj laanwj merged commit fa7f4f5 into bitcoin:master Jun 8, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jun 8, 2016

Merge #8153: [rpc] fundrawtransaction feeRate: Use BTC/kB
fa7f4f5 [rpc] fundrawtransaction feeRate: Use BTC/kB (MarcoFalke)
faf82e8 [rpc] fundrawtransaction: Fix help text and interface (MarcoFalke)
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 8, 2016

Member

utACK fa7f4f5

Member

laanwj commented Jun 8, 2016

utACK fa7f4f5

@laanwj laanwj referenced this pull request Jun 8, 2016

Closed

TODO for release notes 0.13.0 #7678

14 of 16 tasks complete

@MarcoFalke MarcoFalke deleted the MarcoFalke:Mf1606-univalAny branch Jun 8, 2016

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge bitcoin#8153: [rpc] fundrawtransaction feeRate: Use BTC/kB
fa7f4f5 [rpc] fundrawtransaction feeRate: Use BTC/kB (MarcoFalke)
faf82e8 [rpc] fundrawtransaction: Fix help text and interface (MarcoFalke)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge bitcoin#8153: [rpc] fundrawtransaction feeRate: Use BTC/kB
fa7f4f5 [rpc] fundrawtransaction feeRate: Use BTC/kB (MarcoFalke)
faf82e8 [rpc] fundrawtransaction: Fix help text and interface (MarcoFalke)

codablock added a commit to codablock/dash that referenced this pull request Dec 22, 2017

Merge bitcoin#8153: [rpc] fundrawtransaction feeRate: Use BTC/kB
fa7f4f5 [rpc] fundrawtransaction feeRate: Use BTC/kB (MarcoFalke)
faf82e8 [rpc] fundrawtransaction: Fix help text and interface (MarcoFalke)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment