Override Fee Overpayment Protection via RPC

[adamPetho]

Fixes: #12118

I went with this comment #12118 (comment), so this overriding "mechanism" only works via RPC for now.

How to test:

1. Call the build RPC endpoint with a small outgoing amount and huge fees.
2. See TransactionFeeOverpaymentException exception.
3. Call buildunsafetransaction endpoint with a small outgoing amount and huge fees.
4. See no exceptions and a successfully built transaction.

@turbolay @MarnixCroes Could you guys test this please? Much appreciated!!

[MarnixCroes]

cACK
tested, it works

Should it also be added to send method?

[lontivero]

The problem with this approach is that it makes the array representation impossible to use. Let me explain: in the early days this method looked like this:

public string BuildTransaction(PaymentInfo[] payments, OutPoint[] coins, int feeTarget, string password = null)

This is perfectly okay because you can call the method using the json object representation and the json array representation. For example, both calls below just worked equally well:

$ curl -s \
 -H -- 'content-type: text/plain;' \
 --binary-data  '{"jsonrpc":"2.0","id":"1","method":"send", "params": { "payments":[ {"sendto": "tb1qgvnht40a08gumw32kp05hs8mny954hp2snhxcz", "amount": 15000, "label": "David" }], "coins":[{"transactionid":"ab83d9d0b2a9873b8ab0dc48b618098f3e7fbd807e27a10f789e9bc330ca89f7", "index":0}], "feeRate":234 }}' \
  http://127.0.0.1:37128/<wallet-name> 

And

$ curl -s \
 -H -- 'content-type: text/plain;' \
 --binary-data  '{"jsonrpc":"2.0","id":"1","method":"send", "params": [ [ {"sendto": "tb1qgvnht40a08gumw32kp05hs8mny954hp2snhxcz", "amount": 15000, "label": "David" }], [{"transactionid":"ab83d9d0b2a9873b8ab0dc48b618098f3e7fbd807e27a10f789e9bc330ca89f7", "index":0}], 234 ]}' \
  http://127.0.0.1:37128/<wallet-name> 

Now, after #11667 this magic was broken and while the json object representation works perfectly, the array representation doesn't work anymore if we don't pass all parameters. This sucks bad and it was merged only after a year because i didn't find a better solution and I believed it was not a big deal.

After this PR the situation gets a bit worse, and it will become even worse after every additional parameter, so, for all RPC methods devs can use both json representations except for this one (or they are forced to pass all the parameters).

Before continue let me accept that I am the one that ruined it. But now I feel (am) guilty and don't want to let it be worse.
What to do then? The real problem is not, and was never, in the rpc but in the underlying methods. In this case the problem is in Wallet::.BuildTransaction method which receives 6 parameters 2 of which are booleans, that's wrong. After you understand why you should never pass null you understand why you should never pass booleans. Basically a boolean parameter means that the method does two things: one if true and something different if false. In that case it is much better to have two methods:

• BuildTransaction which is the old method with overpayment protection and,
• BuildTransactionWithoutOverpaymentProtection which build a transaction without caring about the fee.

Finally expose both as RPC methods.

[MarnixCroes]

tack f00da95

[turbolay]

tACK
Thanks for the PR.

Should we add a comment in the code for buildunsafetransaction RPC? I know RPC calls are documented, but quickly explaining how this call is unsafe might be beneficial

[MaxHillebrand]

Is it unsafe, or just expensive?

[adamPetho]

Is it unsafe, or just expensive?

This PR doesn't do any magic, only it lets you choose whatever fee/feeRate you want for the transaction.

Let's say, I make a transaction: 0.01 BTC goes to my friend and 0.99 BTC goes to the miners as fee.

Do you consider this transaction unsafe or just expensive?
For me, it's both + it's stupid. But it's still a valid transaction.

When the fees are high like now, it can happen that the user wants to send a small amount with high fee, but the Tx building fails because of this safety check in TransactionFactory

// if the fee is more than 2x of the outgoing amount, then throw exc.
if (feePercentage > 100)
{
	throw new TransactionFeeOverpaymentException(feePercentage);
}

This PR aims to go around this check and let the tx be built.

[adamPetho]

Anything else is missing from this PR?

[MarnixCroes]

re-ack 7586511

[molnard]

cACK, LGTM - except one.

[molnard]

code ACK

[molnard]

@MarnixCroes @yahiheb can you test this by using the RPC?

[MarnixCroes]

tack 1cf1372