RPC: Introduce -rpcamountdecimals for the RPC to use other units than BTC #9882

jtimon commented at 12:12 AM on February 28, 2017: contributor

Replaces #9855. Inspired by #3759 .

RPC: Introduce -rpcamountdecimals for the RPC to use other units than BTC 14f5c79676

QA: Use -rpcamountdecimals in rawtransactions.py ee5f0701b2

fanquake added the label RPC/REST/ZMQ on Feb 28, 2017

gmaxwell commented at 1:00 AM on February 28, 2017: contributor

I don't understand how this is intended to be remotely safe? RPC invisibly changing its units out from under callers sounds like a receipt for disaster.

jtimon commented at 1:08 AM on February 28, 2017: contributor

It was @laanwj 's suggestion in #9855 (comment) Well, #3759 is different, but the same as this in that sense. Do you have other suggestions or do you think we should stick to BTC as unit for the RPC forever?

Honestly my preference would be to break the API and only allow satoshis, but as said in #9855 that would also require many changes on the python tests.

luke-jr commented at 1:32 AM on February 28, 2017: member

I also prefer to just break API and use satoshis everywhere. We've wanted to use satoshis for several years now and keep getting hung up on compatibility. If we'd just changed it to begin with, we'd be long over it by now...

dcousens commented at 2:04 AM on February 28, 2017: contributor

Deprecation of non-satoshi methods would be the only safe option IMHO, with new methods under different names.

#3759 (comment)

sipa commented at 2:16 AM on February 28, 2017: member

I don't think that amounts-in-satoshis-only is necessarily the solution we'd want, even if we could start from scratch.

JSON does not have a concept of integers or doubles, only of numbers. At least for some JSON libraries, using integral number wouldn't be any better than the BTC numbers now.

If anything, my preference would be using strings (in BTC) for amounts, which is already possible (though only for input).

jtimon commented at 2:20 AM on February 28, 2017: contributor

Assuming that we were starting from scratch, why do you prefer using strings in BTC over strings in satoshis?

sipa commented at 2:24 AM on February 28, 2017: member

The currency is BTC, not satoshi. The fact that internally numbers are represented as multiples of 0.00000001 BTC is not necessarily something we should expose (unless there is another good reason - like avoiding rounding - but that isn't an issue when you're using strings already).

sipa commented at 2:27 AM on February 28, 2017: member

To be clear: that's a very minor and unimportant preference. Using satoshis is perfectly reasonable, but I don't think it's so obviously a better choice that it's worth switching to solely that.

dcousens commented at 2:36 AM on February 28, 2017: contributor

@sipa I think the "better choice" reasoning is that most libraries interacting with Bitcoin typically operate on satoshis, externally and internally, and thus it has been assumed as the typical inter-op unit. [in my experience]

That at least, is why it is my preferred unit.

How it is encoded in JSON, String or Number, is simply a matter of what expectations are are made of the JSON parser.

jtimon commented at 2:38 AM on February 28, 2017: contributor

As a disclaimer, I'm doing this because I want to do it in elements. With multiple assets, using always 8 decimals seems more arbitrary. See https://github.com/ElementsProject/elements/pull/132 Of course, the fact that I want it for elements doesn't mean we want it for Bitcoin, but I remembered the satoshi vs BTC discussion and ideally I would prefer to do on elements whatever is more upstreamable to Bitcoin or at least less different from it (for maintainability reasons).

EDIT: Even if this can't be merged I greatly appreciate everyone's feedback, thanks.

jonasschnelli commented at 9:25 AM on February 28, 2017: contributor

I agree with @sipa. Also, I think a such radical API change would be dangerous. It could make sense if we would introduce a co-existign v2 API (where you may can disabled the v1 API). But to do that for just the floating point issue (that has no guarantees to make it safer as @sipa stated) is probably not worth the effort.

in src/rpc/server.cpp:None in ee5f0701b2

 146 | @@ -137,8 +147,9 @@ UniValue ValueFromAmount(const CAmount& amount)
 147 |  {
 148 |      bool sign = amount < 0;
 149 |      int64_t n_abs = (sign ? -amount : amount);
 150 | -    int64_t quotient = n_abs / COIN;
 151 | -    int64_t remainder = n_abs % COIN;
 152 | +    unsigned int nRpcUnit = pow(10, nRPCAmountDecimals);

MarcoFalke commented at 10:39 AM on February 28, 2017:

Please don't use floating point calculations.

jtimon commented at 8:30 PM on February 28, 2017:

should I implement my own pow function for unsigned integers?

MarcoFalke commented at 8:58 PM on February 28, 2017:

Yes, floating point calculations tend to be unpredictable. You could maybe use

static constexpr std::array<CAmount,9> pow_10 {{1,10,100,1000,10000,100000,1000000,10000000,100000000}};
nRpcUnit = pow_10.at(nRPCAmountDecimals);

or come up with something else.

laanwj commented at 10:50 AM on February 28, 2017: member

I don't think that amounts-in-satoshis-only is necessarily the solution we'd want, even if we could start from scratch. JSON does not have a concept of integers or doubles, only of numbers. At least for some JSON libraries, using integral number wouldn't be any better than the BTC numbers now. If anything, my preference would be using strings (in BTC) for amounts, which is already possible (though only for input).

Fully agree here with @sipa. I said this over and over. Satoshis as units is not going to solve anything and risks a 8-magnitude blowup of spends when used in error. Switching to strings (as an option) for AmountToValue doesn't have this issue as it'll just crash clients that cannot cope with it.

kallewoof commented at 6:12 PM on February 28, 2017: member

I personally think we should phase out BTC values completely, as there is an inconsistency when juggling the raw values of a transaction vs using the RPC to manipulate said transaction. There may be others as well, but I'm not sure.

As a middle ground, we could introduce a suffix feature which makes the reference explicit, since we're leaning towards strings anyway.

"123" = whatever the current type is (BTC or sat)
"123btc" = 123 bitcoins
"123sat" = 123 satoshis

If enough users were encouraged to start using these, the impact of streamlining/unifying later would be much lower.

TheBlueMatt commented at 6:36 PM on February 28, 2017: member

Yea, I'm still super not a fan of doing this on a global, per-node-by-config-option basis. Isn't this kind of thing exactly what named arguments is for? Just add a global named argument that lets you pick the encoding you want.

jtimon commented at 8:35 PM on February 28, 2017: contributor

@TheBlueMatt Sorry, I don't know what you mean by "a global named argument". I also need to learn how to do named arguments, I was trying in https://github.com/bitcoin/bitcoin/pull/9855/commits/f799d8b01d4bf475cef26db061f94ea9a879ab23 but that (and some variants like adding the other 2 missing args for signrawtransaction) didn't seem to work.

jtimon commented at 8:37 PM on February 28, 2017: contributor

@laanwj isn't using strings already possible? I thought it was possible and this didn't change that fact, simply the string with satoshis doesnt have decimals and the one with BTC does. I should test it more or maybe I'm missing something.

MarcoFalke added the label Brainstorming on Feb 28, 2017

instagibbs commented at 12:39 AM on March 1, 2017: member

@jtimon seems to mean "let -decimalcount=<n> work for any rpc command so you don't have to scatter it everywhere".

sradforth commented at 10:05 PM on March 1, 2017: none

My +1 is for using satoshi's everywhere, deprecate all API calls that return BTC and create a few new API calls for the new formatted calls. E.g. setTxFee gets deprecated and a new API call of setTxFeeSatoshisPerByte is added.

It seems clear the reason Satoshi used integers and 2,100,000,000,000,000 satoshis was to fit within the 53 bits mantissa of double floats to avoid any risk of rounding errors or overflows while giving the maximum range. Given Javascript uses these double precision floats I'm all for the API being consistent in using Satoshis, in fact just today I encountered a rounding error glitch due to estimateFees API call returning BTC rather than satoshis.

da2ce7 commented at 10:09 AM on March 2, 2017: none

I have the same view as @sradforth . All of the Bitcoin Ecosystem use Satoshi as the unit. It is the de-facto standard. I don't see what is gained from doing the 'technically correct, but different' behaviour.

Strings vs Numbers? I guess the question is: is "2,100,000,000,000,000" unsafe in any commonly used JSON lib?

laanwj commented at 10:39 AM on March 2, 2017: member

@laanwj isn't using strings already possible? I thought it was possible and this didn't change that fact, simply the string with satoshis doesnt have decimals and the one with BTC does. I should test it more or maybe I'm missing something.

Passing strings in is possible (so ValueToAmount), getting them out (so AmountToValue) isn't yet. This would need a setting.

jtimon commented at 10:05 PM on March 6, 2017: contributor

Well, it seems we should first decide whether we ever plan to break the API and use satoshis everywhere or not. Something like this or "a global named argument" would be just temporary arrangements to that that in a slower and less disruptive way. Thanks everyone for their thoughts on the matter. Closing.

jtimon closed this on Mar 6, 2017

MarcoFalke locked this on Sep 8, 2021