Closes #3110. Ensure user can see error message about absurdly high fees. #3111

bitcartel · 2018-03-23T06:58:45Z

No description provided.

daira

utACK modulo comment.

daira · 2018-03-23T09:29:12Z

qa/rpc-tests/wallet.py

+            outputs = {}
+            utxo = node0utxos[0]
+            inputs.append({ "txid" : utxo["txid"], "vout" : utxo["vout"]})
+            outputs[self.nodes[2].getnewaddress("")] = Decimal(0.00001000)


This should be Decimal("0.00001000"), otherwise precision is lost in the value passed into the constructor:

>>> from decimal import Decimal >>> Decimal("0.00001000") Decimal('0.00001000') >>> Decimal(0.00001000) Decimal('0.000010000000000000000818030539140313095458623138256371021270751953125')

Fix force pushed.

daira · 2018-03-23T09:32:37Z

Note that I did not test the user interface behaviour; I only read the code.

str4d · 2018-03-26T13:10:25Z

src/main.cpp

+            string errmsg = strprintf("absurdly high fees %s, %d > %d",
+                             hash.ToString(),
+                             nFees, ::minRelayTxFee.GetFee(nSize) * 10000);
+            return state.DoS(0, error(("AcceptToMemoryPool: " + errmsg).c_str()), REJECT_ABSURDLYHIGHFEE, errmsg);


Use state.Error() instead, which won't ever propagate over the network (matching the existing behaviour of this code), but can be used to return error messages to the caller. It also matches the expected semantics (the transaction is not inherently invalid, which is what is indicated when using state.DoS()).

Hmm. REJECT_ABSURDLYHIGHFEE is similar to REJECT_INSUFFICIENTFEE which uses state.DoS():

return state.DoS(0, error("AcceptToMemoryPool: not enough fees %s, %d < %d", hash.ToString(), nFees, txMinFee), REJECT_INSUFFICIENTFEE, "insufficient fee");

In both cases, the transactions themselves are valid and could be mined into a block. If local rules (a form of "emergent consensus") allow for insufficient fee, shouldn't we treat an absurdly high fee the same way?

Insufficient fee is a node / standard rule, yes - but one that is communicated over the network precisely because it affects remote users (particularly if the local node has enabled DoS-prevention mechanisms, some of which I think we have, and some of which I know are added in upstream's 0.12).

By contrast, "absurdly high fee" has only ever been a local user protection. There is no rational reason for miners to reject high-fee transactions, and if we do in fact desire a decentralised mining state, no rational reason for any node to reject high-fee transactions. If we want to change this (and explicitly start rejecting high-fee transactions over the network, which we do not currently do), we should do so in its own issue, not as a side-effect of this PR (which is about exposing the error message to the local user).

Changed to state.Error to unblock this PR.

str4d · 2018-03-26T13:13:27Z

src/consensus/validation.h

@@ -17,6 +17,7 @@ static const unsigned char REJECT_NONSTANDARD = 0x40;
 static const unsigned char REJECT_DUST = 0x41;
 static const unsigned char REJECT_INSUFFICIENTFEE = 0x42;
 static const unsigned char REJECT_CHECKPOINT = 0x43;
+static const unsigned char REJECT_ABSURDLYHIGHFEE = 0x44;


Remove this (it becomes unnecessary).

str4d · 2018-03-26T13:20:44Z

qa/rpc-tests/wallet.py

+            outputs = {}
+            utxo = node0utxos[0]
+            inputs.append({ "txid" : utxo["txid"], "vout" : utxo["vout"]})
+            outputs[self.nodes[2].getnewaddress("")] = Decimal("0.00001000")


It's not clear here what fee this results in. Maybe add a comment about the size of the input? Or calculate the value output based on the input value being used.

Added comment and updated test so its clearer.

str4d · 2018-03-26T13:22:31Z

qa/rpc-tests/wallet.py

+            signed_tx = self.nodes[0].signrawtransaction(raw_tx)
+            self.nodes[0].sendrawtransaction(signed_tx["hex"])
+        except JSONRPCException,e:
+            errorString = e.error['message']


This will currently result in the test passing if the error is thrown by any of the called methods, not just sendrawtransaction, which might not be what we want to test.

mdr0id · 2018-03-26T21:03:13Z

Tested ACK:

➜  zcash git:(3110_high_fee_error_reporting) ✗ ./qa/pull-tester/rpc-tests.sh wallet --noclean
=== Running testscript wallet.py ===
Initializing test directory /tmp/testXkbJH0
Mining blocks...
Stopping nodes
Tests successful
--- Success: wallet.py ---

Tests completed: 1
successes 1; failures: 0

Fails for values less than Decimal("1.0") :

 ➜  zcash git:(3110_high_fee_error_reporting) ✗ ./qa/pull-tester/rpc-tests.sh wallet --noclean
=== Running testscript wallet.py ===
Initializing test directory /tmp/testZ_N9jQ
Mining blocks...
Assertion failed: 
  File "/home/marsh/zcash_dev_hub/zc_3110/zcash/qa/rpc-tests/test_framework/test_framework.py", line 121, in main
    self.run_test()
  File "/home/marsh/zcash_dev_hub/zc_3110/zcash/qa/rpc-tests/wallet.py", line 110, in run_test
    assert("900000000 > 190000" in errorString)
Stopping nodes
Failed
!!! FAIL: wallet.py !!!

Tests completed: 1
successes 0; failures: 1

Failing tests: wallet.py

bitcartel · 2018-03-27T05:51:19Z

@mdr0id As expected. The test picks out an utxo of value 10.0 to spend from so we can assert on the contents of the error message. It might seem inflexible from the point of view of writing tests, but it can help alert us to any changes to utxo generation and absurd/estimated fee calculation.

str4d

utACK

str4d · 2018-03-27T12:51:36Z

src/main.cpp

+        if (fRejectAbsurdFee && nFees > ::minRelayTxFee.GetFee(nSize) * 10000) {
+            string errmsg = strprintf("absurdly high fees %s, %d > %d",
+                             hash.ToString(),
+                             nFees, ::minRelayTxFee.GetFee(nSize) * 10000);


Nit: these two lines didn't need to change their indentation.

Updated: moved over to align neatly under "

str4d · 2018-03-27T12:53:05Z

src/main.cpp

+            string errmsg = strprintf("absurdly high fees %s, %d > %d",
+                             hash.ToString(),
+                             nFees, ::minRelayTxFee.GetFee(nSize) * 10000);
+            LogPrint("mempool", errmsg.c_str());


Do we usually log without the function name prefix?

It's inconsistent. Code block above does not log function name, but in some other places it is.

str4d · 2018-03-27T12:54:45Z

qa/rpc-tests/wallet.py

+        for utxo in node2utxos:
+            if utxo["amount"] == Decimal("10.0"):
+                break
+        assert_equal(utxo["amount"], Decimal("10.0"))


It somewhat irks me that Python leaves the last iteration of a loop in scope. Handy, yes, but...

Yes! I think this is related to another annoyance of Python:

for i in range(3): print(i) print(i)

produces this output:

0 1 2 2

It seems like the final value of i should be 3 (as in C), not 2. If it were 3, then you'd know you didn't break out of the loop early (not being done in this example, but in general).

…igh fees.

bitcartel · 2018-03-27T17:27:05Z

@zkbot r+

zkbot · 2018-03-27T17:27:06Z

📌 Commit 8b15afd has been approved by bitcartel

…artel Closes #3110. Ensure user can see error message about absurdly high fees.

zkbot · 2018-03-27T17:33:56Z

⌛ Testing commit 8b15afd with merge 99b6f76...

zkbot · 2018-03-27T20:10:38Z

☀️ Test successful - pr-merge
Approved by: bitcartel
Pushing 99b6f76 to master...

str4d · 2018-04-05T15:10:30Z

I just re-discovered bitcoin/bitcoin#5913 which does the same thing upstream. They get around the P2P error code issue by using the two-byte code 0x100, which will never propagate over the network.

ZIP 239 preparations 4 Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#5913 - Replaces #3111. - Includes an extra commit included by upstream in the merge outside the PR. - bitcoin/bitcoin#6519 - bitcoin/bitcoin#7179 - bitcoin/bitcoin#7853 - bitcoin/bitcoin#7960

bitcartel added A-wallet Area: Wallet I-error-handling Problems and improvements related to error handling A-rpc-interface Area: RPC interface labels Mar 23, 2018

bitcartel added this to the v1.1.0 milestone Mar 23, 2018

bitcartel self-assigned this Mar 23, 2018

bitcartel requested review from str4d and mdr0id March 23, 2018 06:59

bitcartel mentioned this pull request Mar 23, 2018

TX decode failed. Code:-22 (due to absurdly high fee when using create/sendrawtransaction) #3110

Closed

daira approved these changes Mar 23, 2018

View reviewed changes

bitcartel force-pushed the 3110_high_fee_error_reporting branch from 4f6110a to 3e991a9 Compare March 23, 2018 22:26

str4d requested changes Mar 26, 2018

View reviewed changes

bitcartel force-pushed the 3110_high_fee_error_reporting branch from 3e991a9 to 5190a0f Compare March 26, 2018 19:44

bitcartel force-pushed the 3110_high_fee_error_reporting branch from 5190a0f to 2d8cf3f Compare March 27, 2018 06:20

str4d approved these changes Mar 27, 2018

View reviewed changes

Closes zcash#3110. Ensure user can see error message about absurdly h…

8b15afd

…igh fees.

bitcartel force-pushed the 3110_high_fee_error_reporting branch from 2d8cf3f to 8b15afd Compare March 27, 2018 17:23

zkbot added a commit that referenced this pull request Mar 27, 2018

Auto merge of #3111 - bitcartel:3110_high_fee_error_reporting, r=bitc…

99b6f76

…artel Closes #3110. Ensure user can see error message about absurdly high fees.

mdr0id approved these changes Mar 27, 2018

View reviewed changes

zkbot merged commit 8b15afd into zcash:master Mar 27, 2018

zkbot added this to Complete in Continuous Improvement via automation Mar 27, 2018

str4d mentioned this pull request Aug 13, 2021

ZIP 239 preparations 4 #5275

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closes #3110. Ensure user can see error message about absurdly high fees. #3111

Closes #3110. Ensure user can see error message about absurdly high fees. #3111

bitcartel commented Mar 23, 2018

daira left a comment

daira Mar 23, 2018

bitcartel Mar 23, 2018

daira commented Mar 23, 2018

str4d Mar 26, 2018

bitcartel Mar 26, 2018

str4d Mar 26, 2018

bitcartel Mar 27, 2018

str4d Mar 26, 2018

bitcartel Mar 27, 2018

str4d Mar 26, 2018

bitcartel Mar 26, 2018

str4d Mar 26, 2018

bitcartel Mar 26, 2018

mdr0id commented Mar 26, 2018

bitcartel commented Mar 27, 2018

str4d left a comment

str4d Mar 27, 2018

bitcartel Mar 27, 2018

str4d Mar 27, 2018

bitcartel Mar 27, 2018

str4d Mar 27, 2018

LarryRuane Apr 17, 2018

bitcartel commented Mar 27, 2018

zkbot commented Mar 27, 2018

zkbot commented Mar 27, 2018

zkbot commented Mar 27, 2018

str4d commented Apr 5, 2018

Closes #3110. Ensure user can see error message about absurdly high fees. #3111

Closes #3110. Ensure user can see error message about absurdly high fees. #3111

Conversation

bitcartel commented Mar 23, 2018

daira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daira commented Mar 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdr0id commented Mar 26, 2018

bitcartel commented Mar 27, 2018

str4d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitcartel commented Mar 27, 2018

zkbot commented Mar 27, 2018

zkbot commented Mar 27, 2018

zkbot commented Mar 27, 2018

str4d commented Apr 5, 2018