revault / practical-revault

Version 0 specifications for a Revault deployment
Creative Commons Attribution 4.0 International
33 stars 9 forks source link

Unvault tx fixed feerate of 253perkw isn't a good idea #15

Closed darosior closed 3 years ago

darosior commented 4 years ago

Not crucial security-wise as it's not a revocation tx (to which we attach fee-bumping inputs so it's fine), but if the rolling feerate of nodes' mempools get above that (as it is at the time i'm writing this) then the unvault transaction won't be relayed (feefilter protocol message).

TL;DR: unvault tx fees should not be static (or at least not that low but it does not count as a solution)

darosior commented 4 years ago

Hmm since the cpfp output is immediately valid (no CSV) i wonder if we could be smart and make bitcoind evaluate the feerate of the whole package by firstly giving it the (orphan) child tx, and then the (below mempool min fee) parent tx. This requires using the P2P interface, but i need to check if bitcoind would actually evaluate the package feerate in this case.

darosior commented 3 years ago

Ok, it did not work out. bitcoind is (expectedly..) not considering the orphan child when sending the low-feerate parent.

darosior commented 3 years ago

Here is how i tested it, FWIW:

diff --git a/test/functional/mempool_limit.py b/test/functional/mempool_limit.py
index 39035f7cb..bd91a8c36 100755
--- a/test/functional/mempool_limit.py
+++ b/test/functional/mempool_limit.py
@@ -32,7 +32,7 @@ class MempoolLimitTest(BitcoinTestFramework):
         assert_equal(self.nodes[0].getmempoolinfo()['mempoolminfee'], Decimal('0.00001000'))

         txids = []
-        utxos = create_confirmed_utxos(relayfee, self.nodes[0], 91)
+        utxos = create_confirmed_utxos(relayfee, self.nodes[0], 100)

         self.log.info('Create a mempool tx that will be evicted')
         us0 = utxos.pop()
@@ -70,5 +70,42 @@ class MempoolLimitTest(BitcoinTestFramework):
         txFS = self.nodes[0].signrawtransactionwithwallet(txF['hex'])
         assert_raises_rpc_error(-26, "mempool min fee not met", self.nodes[0].sendrawtransaction, txFS['hex'])

+
+        from test_framework.messages import CTransaction, FromHex
+        from test_framework.p2p import P2PDataStore
+
+        self.log.info('Create a mempool tx at 253sat/kw')
+        u = utxos.pop()
+        inputs = [{ "txid": u["txid"], "vout": u["vout"] }]
+        outputs = { self.nodes[0].getnewaddress(): 0.001 }
+        tx = self.nodes[0].createrawtransaction(inputs, outputs)
+        tx = self.nodes[0].fundrawtransaction(tx, {'feeRate': 0.00001})
+        tx = self.nodes[0].signrawtransactionwithwallet(tx['hex'])
+        assert_raises_rpc_error(-26, "mempool min fee not met", self.nodes[0].sendrawtransaction, tx['hex'])
+
+        self.log.info('Create a child tx with a huge feerate to CPFP')
+        parent_txid = self.nodes[0].decoderawtransaction(tx["hex"])["txid"]
+        inputs = [{ "txid": parent_txid, "vout": 0 }]
+        outputs = { self.nodes[0].getnewaddress(): 0.00001 }
+        child_tx = self.nodes[0].createrawtransaction(inputs, outputs)
+        child_tx = self.nodes[0].signrawtransactionwithwallet(child_tx)
+        assert_raises_rpc_error(-25, "bad-txns-inputs-missingorspent",
+                                self.nodes[0].sendrawtransaction, child_tx['hex'])
+
+        self.log.info('Send it via P2P: first the high-paying orphan, then the '
+                      '253perw parent.')
+        child_ctx = FromHex(CTransaction(), child_tx["hex"])
+        parent_ctx = FromHex(CTransaction(), tx["hex"])
+        self.nodes[0].add_p2p_connection(P2PDataStore())
+        self.nodes[0].p2ps[0].send_txs_and_test([child_ctx], self.nodes[0],
+                                                # False because the orphan
+                                                # won't be part of
+                                                # `getrawmempool`
+                                                success=False)
+        self.nodes[0].p2ps[0].send_txs_and_test([parent_ctx], self.nodes[0],
+                                                success=True)
+
darosior commented 3 years ago

Ok, so according to https://www.statoshi.info/dashboard/db/memory-pool i think a static 125sat/vByte is reasonable (i mean it's not but we can't do otherwise, i could even argue that we set 250 but one could say that we can update it as we go and urge people to update.. Or that we'd have package relay at this point! ).

darosior commented 3 years ago

This also means we'd not have to use CPFP in most cases which also lowers the cost

darosior commented 3 years ago

We settled upon:

darosior commented 3 years ago

Reopening until we close it into the mesages spec.