InstantSend overhaul #1288

UdjinM6 · 2017-01-28T07:07:22Z

This is a complete refactoring of InstantSend and implementation of multi-quorum idea i.e. to use a separate masternode quorum to vote on every outpoint, not only on the tx itself which should dramatically reduce the possibility of conflicts (and save resources needed to resolve them). Using multi-quorum on the other hand can potentially rise resource usage for quorum (for huge txes). With this in mind, max number of inputs for InstantSend transaction was limited to 10 for now and final InstantSend fee is now also calculated as number of inputs * fee per input where fee per input is the same as current INSTANTSEND_MIN_FEE (i.e. 0.001 * COIN) to dis-incentivize huge txes economically.

As part of re-implementation this also changes buffer behavior to expire lock request candidates/votes based on the block corresponding tx was confirmed rather than on the block lock candidate/vote was created and fixes race condition "block vs lock".

NOTE: PROTOCOL_VERSION and MIN_INSTANTSEND_PROTO_VERSION were bumped because TXLOCKVOTE p2p-message now has new format.

+ cleanup for IS and partial protobump

- allow InstantSend tx to have 10 inputs max - store many unique tx hashes in mapVotedOutpoints - more checks in AcceptToMemoryPoolWorker (moved from ProcessMessage + CTxLockRequest(tx).IsValid() )

- let multiple lock candidates compete for votes - fail to vote on the same outpoint twice early

- notify CInstantSend on UpdatedBlockTip -> remove cs_main from CheckAndRemove() - notify CInstantSend on SyncTransaction -> count expiration block starting from the block corresponding tx was confirmed instead of the block lock candidate/vote was created - fixed few locks

tgflynn

Great work.

Please check a few code issues/comments.

I think biggest points are the logic of the argument to CTxLockRequest::IsValid and limiting the depth of recursion in ResolveConflicts.

tgflynn · 2017-01-28T08:13:42Z

src/instantx.cpp

+            LogPrint("instantsend", "CTxLockRequest::IsValid -- Failed to find UTXO %s\n", txin.prevout.ToStringShort());
+            // Normally above sould be enough, but in case we are reprocessing this because of
+            // a lot of legit orphan votes we should also check already spent outpoints.
+            if(!fRequireUnspent) return false;


Shouldn't this be "if(fRequireUnspent)" ?

👍 see above

tgflynn · 2017-01-28T09:11:41Z

src/instantx.cpp

+    // Normally we should require all outpoints to be unspent, but in case we are reprocessing
+    // because of a lot of legit orphan votes we should also check already spent outpoints.
+    uint256 txHash = txLockRequest.GetHash();
+    if(!txLockRequest.IsValid(IsEnoughOrphanVotesForTx(txLockRequest))) return false;


Seems like it should be: "IsValid(!IsEnoughOrphanVotesForTx)"

Hah, good catch! I actually started with fAllowSpent in my draft and changed it to fRequireUnspent later but didn't change the param and if it inside of the function to match this, so it works but looks confusing now. Will fix!

tgflynn · 2017-01-28T15:43:58Z

src/instantx.cpp

+        } else if (mempool.mapNextTx.count(txin.prevout)) {
+            // conflicting with tx in mempool
+            fMempoolConflict = true;
+            const CTransaction *ptxConflicting = mempool.mapNextTx[txin.prevout].ptx;


Looks like the rest of the code in this block is only for printing debug messages, is that right ?

Yep, could break here instead but I'm just trying to be very verbose about conflicting tx and what's going to happen next. Actually this block of code needs a bit more work, will fix.

tgflynn · 2017-01-28T15:48:23Z

src/instantx.cpp

+            //
+            LogPrintf("CTxLockRequest::ResolveConflicts -- Failed to find UTXO %s - disconnecting tip...\n", txin.prevout.ToStringShort());
+            DisconnectBlocks(1);
+            // Recursively check at "new" old height. Conflicting tx should be rejected by AcceptToMemoryPool.


I think it would be safer to limit the depth of recursion to a "reasonable" number of blocks (maybe 6) than to allow the entire chain to be unrolled.

Makes sense to limit depth here. Not sure about 6, probably should use nInstantSendKeepLock to make sure that we didn't miss it somewhere in the past.

tgflynn · 2017-01-28T15:53:12Z

src/instantx.cpp

+    // forcing external script notification.
+    if(IsInstantSendReadyToLock(txHash)) {
+        if(ResolveConflicts(txLockCandidate)) {
+            // the order here matters


"The order matters", but why this order. Seems like normally you would want to establish the locks before sending notifications.

Actually it's just wrong :/ will fix 👍

tgflynn · 2017-01-28T16:12:15Z

src/instantx.cpp

        }
-        return false;
+        // we have votes by other masternodes only (so far), let's continue and see who will win


I think vote should be added to matching mapVotedOutpoints set here.

tgflynn · 2017-01-28T16:14:35Z

src/instantx.cpp

+        if(ResolveConflicts(it->second)) {
+            // the order here matters
+            UpdateLockedTransaction(it->second);
+            if(!IsLockedInstantSendTransaction(txHash)) {


If the tx is already locked, it seems like we should return earlier than this (ie. why call ResolveConflicts/UpdateLockedTransaction ?).

Will fix this one too

tgflynn · 2017-01-28T16:18:22Z

src/instantx.cpp

+
+void CInstantSend::AcceptLockRequest(const CTxLockRequest& txLockRequest)
+{
+    mapLockRequestAccepted.insert(make_pair(txLockRequest.GetHash(), txLockRequest));


Missing mutex lock.

tgflynn · 2017-01-28T16:18:36Z

src/instantx.cpp

+
+void CInstantSend::RejectLockRequest(const CTxLockRequest& txLockRequest)
+{
+    mapLockRequestRejected.insert(make_pair(txLockRequest.GetHash(), txLockRequest));


Missing mutex lock.

UdjinM6

Thanks, will fix shortly! 👍

UdjinM6 · 2017-01-28T16:47:30Z

src/instantx.cpp

+    // forcing external script notification.
+    if(IsInstantSendReadyToLock(txHash)) {
+        if(ResolveConflicts(txLockCandidate)) {
+            // the order here matters


Actually it's just wrong :/ will fix 👍

UdjinM6 · 2017-01-28T16:54:21Z

src/instantx.cpp

+    // Normally we should require all outpoints to be unspent, but in case we are reprocessing
+    // because of a lot of legit orphan votes we should also check already spent outpoints.
+    uint256 txHash = txLockRequest.GetHash();
+    if(!txLockRequest.IsValid(IsEnoughOrphanVotesForTx(txLockRequest))) return false;


Hah, good catch! I actually started with fAllowSpent in my draft and changed it to fRequireUnspent later but didn't change the param and if it inside of the function to match this, so it works but looks confusing now. Will fix!

UdjinM6 · 2017-01-28T17:04:32Z

src/instantx.cpp

        }
-        return false;
+        // we have votes by other masternodes only (so far), let's continue and see who will win


UdjinM6 · 2017-01-28T17:26:12Z

src/instantx.cpp

+        if(ResolveConflicts(it->second)) {
+            // the order here matters
+            UpdateLockedTransaction(it->second);
+            if(!IsLockedInstantSendTransaction(txHash)) {


Will fix this one too

UdjinM6 · 2017-01-28T18:10:01Z

src/instantx.cpp

+        } else if (mempool.mapNextTx.count(txin.prevout)) {
+            // conflicting with tx in mempool
+            fMempoolConflict = true;
+            const CTransaction *ptxConflicting = mempool.mapNextTx[txin.prevout].ptx;


Yep, could break here instead but I'm just trying to be very verbose about conflicting tx and what's going to happen next. Actually this block of code needs a bit more work, will fix.

UdjinM6 · 2017-01-28T22:49:33Z

src/instantx.cpp

+            //
+            LogPrintf("CTxLockRequest::ResolveConflicts -- Failed to find UTXO %s - disconnecting tip...\n", txin.prevout.ToStringShort());
+            DisconnectBlocks(1);
+            // Recursively check at "new" old height. Conflicting tx should be rejected by AcceptToMemoryPool.


Makes sense to limit depth here. Not sure about 6, probably should use nInstantSendKeepLock to make sure that we didn't miss it somewhere in the past.

UdjinM6 · 2017-01-28T22:56:53Z

src/instantx.cpp

+
+void CInstantSend::AcceptLockRequest(const CTxLockRequest& txLockRequest)
+{
+    mapLockRequestAccepted.insert(make_pair(txLockRequest.GetHash(), txLockRequest));


UdjinM6 · 2017-01-28T22:57:00Z

src/instantx.cpp

+
+void CInstantSend::RejectLockRequest(const CTxLockRequest& txLockRequest)
+{
+    mapLockRequestRejected.insert(make_pair(txLockRequest.GetHash(), txLockRequest));


UdjinM6 · 2017-01-28T22:57:23Z

src/instantx.cpp

+            LogPrint("instantsend", "CTxLockRequest::IsValid -- Failed to find UTXO %s\n", txin.prevout.ToStringShort());
+            // Normally above sould be enough, but in case we are reprocessing this because of
+            // a lot of legit orphan votes we should also check already spent outpoints.
+            if(!fRequireUnspent) return false;


👍 see above

- fix logic for locking inputs and notifying - see UpdateLockedTransaction, TryToFinalizeLockCandidate - add missing hash inserting in ProcessTxLockVote - add nMaxBlocks param to ResolveConflicts to limit max depth allowed to disconnect blocks recursively - fix false positive mempool conflict - add missing mutex locks - fix fRequireUnspent logic in CTxLockRequest::IsValid

tgflynn

utACK

UdjinM6 added 7 commits January 24, 2017 15:17

Multi-quorum InstantSend, complete refactoring

b29279f

+ cleanup for IS and partial protobump

more changes:

c739f4a

- allow InstantSend tx to have 10 inputs max - store many unique tx hashes in mapVotedOutpoints - more checks in AcceptToMemoryPoolWorker (moved from ProcessMessage + CTxLockRequest(tx).IsValid() )

More changes:

ff65827

- let multiple lock candidates compete for votes - fail to vote on the same outpoint twice early

add comments about nConfirmedHeight

40d7556

Fix "Block vs Lock" edge case

c3b3b3a

Fix "Block vs Lock" edge case, p2

6bd0882

tgflynn suggested changes Jan 28, 2017

View reviewed changes

UdjinM6 commented Jan 28, 2017

View reviewed changes

tgflynn approved these changes Jan 29, 2017

View reviewed changes

UdjinM6 merged commit 60409df into dashpay:v0.12.1.x Jan 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

InstantSend overhaul #1288

InstantSend overhaul #1288

UdjinM6 commented Jan 28, 2017

tgflynn left a comment

tgflynn Jan 28, 2017

UdjinM6 Jan 28, 2017

tgflynn Jan 28, 2017

UdjinM6 Jan 28, 2017

tgflynn Jan 28, 2017

UdjinM6 Jan 28, 2017

tgflynn Jan 28, 2017

UdjinM6 Jan 28, 2017

tgflynn Jan 28, 2017

UdjinM6 Jan 28, 2017

tgflynn Jan 28, 2017

UdjinM6 Jan 28, 2017

tgflynn Jan 28, 2017

UdjinM6 Jan 28, 2017

tgflynn Jan 28, 2017

UdjinM6 Jan 28, 2017

tgflynn Jan 28, 2017

UdjinM6 Jan 28, 2017

UdjinM6 left a comment

UdjinM6 Jan 28, 2017

UdjinM6 Jan 28, 2017

UdjinM6 Jan 28, 2017

UdjinM6 Jan 28, 2017

UdjinM6 Jan 28, 2017

UdjinM6 Jan 28, 2017

UdjinM6 Jan 28, 2017

UdjinM6 Jan 28, 2017

UdjinM6 Jan 28, 2017

tgflynn left a comment

InstantSend overhaul #1288

InstantSend overhaul #1288

Conversation

UdjinM6 commented Jan 28, 2017

tgflynn 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UdjinM6 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgflynn left a comment

Choose a reason for hiding this comment