Apply AreSane() checks to the fees from the network. #5481

1. 
   gmaxwell commented at 2:57 AM on December 16, 2014: contributor

   'Sane' was already defined by this code as: fee.GetFeePerK() > minRelayFee.GetFeePerK() * 10000 But sanity was only enforced for data loaded from disk.

   Note that this is a pretty expansive definition of 'sane': A 10 BTC fee is still passes the test if its on a 100kb transaction.

   This prevents a single insane fee on the network from making us reject our stored fee data at start. We still may reject valid saved fee state if minRelayFee is changed between executions.

   This also reduces the risk and limits the damage from a cascading failure where one party pays a bunch of insane fees which cases others to pay insane fees.

2. Apply AreSane() checks to the fees from the network.

   'Sane' was already defined by this code as:
    fee.GetFeePerK() > minRelayFee.GetFeePerK() * 10000
    But sanity was only enforced for data loaded from disk.
   
   Note that this is a pretty expansive definition of 'sane': A 10 BTC
    fee is still passes the test if its on a 100kb transaction.
   
   This prevents a single insane fee on the network from making us reject
    our stored fee data at start.  We still may reject valid saved fee
    state if minRelayFee is changed between executions.
   
   This also reduces the risk and limits the damage from a cascading
    failure where one party pays a bunch of insane fees which cases
    others to pay insane fees.

   6484930690
3. 
   gmaxwell commented at 2:59 AM on December 16, 2014: contributor

   I think in addition to this we really ought to have an absolute maximum fee setting exposed to users... but thats going beyond the narrow bugfixy nature of this change so I didn't do it here.

   The reason I think that is because it's my expirence that people basically spaz out and assume "all their coins" when they don't know in advance of what the fee could be, even when they're engineers or business people who ought to be handling it in a more structured way. ... and this is doubly bad when, in theory, the fee could-- at least in theory-- actually be rather high as is the case with the estimation code.

4. 
   laanwj commented at 6:15 AM on December 16, 2014: member

   utACK

   Re: maximum fees, a hard maximum would be a good precaution but it doesn't solve the uneasiness of 'send the transaction without knowing the fees'. E.g. the GUI handles that by showing the computed fee and having the user confirm the transaction. RPC could grow a similar functionality. For example by returning the raw transaction in a JSON structure w/ some statistics like the fee from send*, then having the client submit it (or even sign+submit) when it agrees. But ofc that's not a quick-fix kind of thing that we can add to 0.10.

5. 
   gmaxwell commented at 7:53 AM on December 16, 2014: contributor

   Makes for a kuldgy interface in the RPC, however. An alternative would be the RPC letting you specify the maximums (ugh, you may have absolute and per kb opinions) and then having the rpc just fail if it can't meet your demands.

   This avoids having to have a callback, or a rawtransaction involving two phase protocol.

6. 
   laanwj commented at 8:03 AM on December 16, 2014: member

   I don't think it's necessarily kludgy. The two-phase protocol is useful for other things, too. E.g. as we discussed before, to create a transaction and lock the inputs but don't submit the transaction yourself, or submit it later (you'd also need a cancel RPC that unlocks the inputs).

   Also the fee may not be the only criterion to decide not to submit a transaction after all. As you say, even the 'fee' constraint may be interpreted in different ways. In my philosophy of doing as little as possible in bitcoind it makes sense to make the user evaluate this instead of build an array of possible checks into the server.

7. 
   gmaxwell commented at 8:04 AM on December 16, 2014: contributor

   Yea, I agree. I'm sold. And we did, indeed, like the {createautoraw ; sign ; send}-ish workflows for other reasons.

8. 
   laanwj commented at 8:09 AM on December 16, 2014: member

   In case you misunderstood, I'm not against also having a hard maximum fee check as a global option, that'd be a small change that still'd be possible to still integrate for 0.10.

9. gmaxwell added this to the milestone 0.10.0 on Dec 16, 2014
10. 
    morcos commented at 4:48 PM on December 16, 2014: member

    I like this. I reviewed the code. If we end up going with #5159 (?) then AreSane was removed anyway as individual transactions can't overly affect the estimate, but I could add it back as a precaution.

11. laanwj added the label Wallet on Dec 17, 2014
12. 
    laanwj commented at 4:14 PM on December 17, 2014: member

    @morcos Most likely we'll switch to the new fee estimation for 0.11. This is meant as a last-minute fix to make 0.10 code sane.

13. laanwj merged this on Dec 23, 2014
14. laanwj closed this on Dec 23, 2014

    ---

15. laanwj referenced this in commit 055f3ae9aa on Dec 23, 2014
16. gmaxwell referenced this in commit 15ad0b54fa on Dec 23, 2014
17. reddink referenced this in commit 99ec743b1d on May 27, 2020
18. MarcoFalke locked this on Sep 8, 2021

Milestone
0.10.0