New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatic InstantSend locks for "simple" transactions #2140

Merged
merged 33 commits into from Sep 26, 2018

Conversation

Projects
None yet
9 participants
@gladcow
Collaborator

gladcow commented Jun 21, 2018

All "simple" transactions (now they are transactions with less than MAX_INPUTS_FOR_AUTO_IX inputs) are locked after creation in wallet (GUI or RPC calls) or after receiving by other node in CInv message.

Such automatic locks are impossible with special InstantSend fees, so these fees are removed (this change requires protocol bump, only new nodes will accept InstantSend with small fee).

Transactions created and sent with rawtransaction RPC API are not locked by sending node (but they will be locked by other nodes after receiving this transaction).

Also, `locktransaction' RPC call was added. It allows any node to lock any transaction.

@UdjinM6 UdjinM6 added this to the 12.4 milestone Jun 21, 2018

@PastaPastaPasta

This comment has been minimized.

PastaPastaPasta commented Jun 21, 2018

@gladcow Can you go into a little bit of detail about what this does? What does a 'lock' in this context mean?

@thephez

This comment has been minimized.

thephez commented Jun 21, 2018

If I'm understanding correctly, it:

  1. Removes the IS fee 🎉
  2. Automatically attempts to IS lock any tx with less than a certain number of inputs. And allows the txlock to be requested by any node that receives a compatible tx (not just the sending one)

If (2) is correct, it means that wallets wouldn't have to be aware of IS to send IS txs (because a receiving node could request the txlock). 😲

@gladcow

This comment has been minimized.

Collaborator

gladcow commented Jun 21, 2018

@paulied , all usual transactions become InstantSend transactions even if they are created by the wallet that doesn't support InstantSend, for example.

@thephez, yes, you are right.)

@PastaPastaPasta

This comment has been minimized.

PastaPastaPasta commented Jun 21, 2018

Are quorums able to do that so quickly at this stage? Our network could get up to 22 tx/seconds. My understanding was that IS resulted in huge bandwidth, and as such could not be on by default.

@thephez

This comment has been minimized.

thephez commented Jun 21, 2018

@paulied There is some info on Discord. Summarizing - basically previous refactoring has been working toward this, intensive testing required, and it probably will not scale to all txs (but future enhancements could make it even more scalable).

@UdjinM6

This comment has been minimized.

UdjinM6 commented Jun 21, 2018

To address concerns about bandwidth - we tested 2MB blocks filled with IS txes on testnet and network handled it just fine. We'll test it again with these changes too of course, but in general, if you are running mainnet full node you should probably be prepared for bandwidth growth anyway.

@Antti-Kaikkonen

This comment has been minimized.

Antti-Kaikkonen commented Jun 21, 2018

Are there any scenarios where the receiver wouldn't be able to lock the received transaction? If the transaction depends on non-locked transactions then can the receiver just lock all the dependencies? I can imagine that this wouldn't work if the dependencies contain too recent coinbase transactions.

@thephez

This comment has been minimized.

thephez commented Jun 22, 2018

@Antti-Kaikkonen

Are there any scenarios where the receiver wouldn't be able to lock the received transaction?

Sure, but a valid transaction would just continue to be a normal transaction (instead of getting "upgraded" to an IS one by the receiver). Similar to how a failed IS request today fails-back to being a regular tx (as long as it meets all the normal tx rules).

@Antti-Kaikkonen

This comment has been minimized.

Antti-Kaikkonen commented Jun 22, 2018

Sure, but a valid transaction would just continue to be a normal transaction

I think that this update is great but it would be even better if the receiver could somehow ensure that all transactions can be locked. Are there any plans to achieve this? For example IS only addresses were discussed at some point.

@gladcow

This comment has been minimized.

Collaborator

gladcow commented Jun 22, 2018

@Antti-Kaikkonen:I think it is better to create special issue with feature request to discuss such opportunity.

@UdjinM6

I'm pretty sure that with changes like that you are effectively removing non-simple IS txes which is probably far too much. Also, introducing another "type" of IS txes should probably be done with some kind of activation because otherwise you'll fail at both validation, voting and verifying your votes on older nodes which opens an attack vector for splitting the network I think.

@@ -934,6 +934,11 @@ std::string CInstantSend::ToString()
return strprintf("Lock Candidates: %llu, Votes %llu", mapTxLockCandidates.size(), mapTxLockVotes.size());
}
bool CInstantSend::IsTrxSimple(const CTransaction &tx)
{
return (tx.vin.size() < MAX_INPUTS_FOR_AUTO_IX);

This comment has been minimized.

@UdjinM6
/// For how long we are going to accept votes/locks
/// after we saw the first one for a specific transaction
static const int INSTANTSEND_LOCK_TIMEOUT_SECONDS = 15;
/// For how long we are going to keep invalid votes and votes for failed lock attempts,
/// must be greater than INSTANTSEND_LOCK_TIMEOUT_SECONDS
static const int INSTANTSEND_FAILED_TIMEOUT_SECONDS = 60;
/// If transaction has less inputs than MAX_INPUTS_FOR_AUTO_IX,

This comment has been minimized.

@UdjinM6
CTransactionRef tx;
uint256 hashBlock;
if (!GetTransaction(hash, tx, Params().GetConsensus(), hashBlock, true))

This comment has been minimized.

@UdjinM6

UdjinM6 Jun 22, 2018

Trying to lock blockchain tx makes no sense imo (and will most likely fail).

LogPrint("instantsend", "CTxLockRequest::IsValid -- did not include enough fees in transaction: fees=%d, tx=%s", nValueOut - nValueIn, ToString());
return false;
}

This comment has been minimized.

@UdjinM6

UdjinM6 Jun 22, 2018

You can't just drop it like that, should check if there is enough fee OR it's a "simple" tx.

CAmount nMinFee = MIN_FEE;
return std::max(nMinFee, CAmount(tx->vin.size() * nMinFee));
}

This comment has been minimized.

@UdjinM6

UdjinM6 Jun 22, 2018

Same, should modify this to handle simple txes correctly instead of dropping this.

@@ -568,9 +568,6 @@ void CoinControlDialog::updateLabels(WalletModel *model, QDialog* dialog)
if (nPayFee > 0 && coinControl->nMinimumTotalFee > nPayFee)
nPayFee = coinControl->nMinimumTotalFee;
// InstantSend Fee
if (coinControl->fUseInstantSend) nPayFee = std::max(nPayFee, CTxLockRequest(txDummy).GetMinFee());

This comment has been minimized.

@UdjinM6

UdjinM6 Jun 22, 2018

This should stay.

@@ -3740,9 +3741,6 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
if (coinControl && nFeeNeeded > 0 && coinControl->nMinimumTotalFee > nFeeNeeded) {
nFeeNeeded = coinControl->nMinimumTotalFee;
}
if(fUseInstantSend) {
nFeeNeeded = std::max(nFeeNeeded, CTxLockRequest(txNew).GetMinFee());
}

This comment has been minimized.

@UdjinM6

UdjinM6 Jun 22, 2018

This should stay.

@gladcow gladcow changed the title from Automatic InstantSend locks for "simple" transactions to [WIP] Automatic InstantSend locks for "simple" transactions Jun 22, 2018

@krishnawhite

This comment has been minimized.

krishnawhite commented Jun 26, 2018

Would this change make the network vulnerable to a denial of service attack where the attacker spams the network with low-fee IS transactions in order to fill the mempool and prevent legitimate transactions?

My understanding is that if the mempool becomes full, under normal circumstances there is a fee mechanism which allows the lowest fee non-IS transactions to be dropped in favour of higher fee transactions - dropping non-IS transactions from the mempool is OK because they are not considered to be confirmed until inclusion in a block. However, InstantSend transactions cannot be dropped from the mempool regardless of the fee paid, because they are considered to be guaranteed as soon as the lock has been issued (at which point they are still in the mempool).

Therefore filling the mempool with low-fee IS transactions would prevent any further transactions - even new transactions that pay a high fee would not be able to displace existing low fee transactions as they are considered locked.

At present, there is a higher fee associated with IS transactions, which makes it costly for an attacker to fill the mempool with IS transactions and keep it full as blocks come in. However this proposed change lowers the IS fee and therefore lowers the cost of the attack.

A very crude approximation of the cost involved goes something like this:

Masternode RAM requirement = 1GB = 1,000,000,000 bytes (mempool capacity).
Assume a transaction size of 250 bytes.
Assume a transaction fee of $0.0006 (the current median).

1,000,000,000 / 250 = 4,000,000 transactions max in the mempool.
4,000,000 * $0.0006 = $2400 to fill the mempool with IS-locked spam transactions.

2MB = 2,000,000 bytes per block.
2,000,000 / 250 = 8000 transactions per block.
8000 * $0.0006 = $4.80 every 2.5 minutes, or $2764 per day to sustain the attack.

So the total cost of this attack is in the region of $2400 initially + $2764 per day unless I am missing something important.

@gladcow

This comment has been minimized.

Collaborator

gladcow commented Jun 26, 2018

@krishnawhite , you are right, now I'm going to check the special mempool conditions to prevent this spam attack.)

@PastaPastaPasta

This comment has been minimized.

PastaPastaPasta commented Jun 26, 2018

@krishnawhite another aspect of this to take into account, is how much dash someone needs to hold in order to execute this attack.

Minimum send amount - 546 duffs. based on this, in order to fill the mempool (assuming 4,000,000 txs is accurate) would be 2,184,000,000 duffs, which is 21.84 Dash.

Now, every 2.5 ish minutes you need to refill after a block goes through, about 8000 txs. So for one block that is 4,368,000 duffs per block. Now, since inputs to an IS need 6 confirmations we multiply that number by six. 26208000 duffs. Or just 0.262 Dash.

Add them together and you find someone would need about 22 dash in holdings to be able to in theory pull off this attack.

A potential fix and mitigation, is to set a minimum value for txs that can be confirmed via IS on a low fee

@gladcow gladcow force-pushed the gladcow:recixlock branch 3 times, most recently from 5019ab7 to 451242e Jul 10, 2018

@gladcow gladcow changed the title from [WIP] Automatic InstantSend locks for "simple" transactions to Automatic InstantSend locks for "simple" transactions Jul 16, 2018

@gladcow gladcow force-pushed the gladcow:recixlock branch from 451242e to 3a75b38 Jul 16, 2018

@UdjinM6

Getting closer :) 👍

See inline comments.

@@ -233,7 +235,7 @@ void CInstantSend::Vote(CTxLockCandidate& txLockCandidate, CConnman& connman)
int nLockInputHeight = nPrevoutHeight + Params().GetConsensus().nInstantSendConfirmationsRequired - 2;
int nRank;
int nMinRequiredProtocol = std::max(MIN_INSTANTSEND_PROTO_VERSION, mnpayments.GetMinMasternodePaymentsProto());
int nMinRequiredProtocol = std::max(MIN_INSTANTSEND_WITHOUT_FEE_PROTO_VERSION, mnpayments.GetMinMasternodePaymentsProto());

This comment has been minimized.

@UdjinM6

UdjinM6 Jul 16, 2018

This will lead to failed "normal" ixes during migration because ranks are going to be messed up for new nodes vs old nodes. Should be reverted imo.

return (mempool.UsedMemoryShare() < AUTO_IX_MEMPOOL_THRESHOLD);
}
bool CInstantSend::IsTrxSimple(const CTransaction &tx)

This comment has been minimized.

@UdjinM6

UdjinM6 Jul 16, 2018

Trx is a bit uncommon abbreviation for transaction. Also, this should probably be a regular non-static function in CTxLockRequest instead, smth like bool CTxLockRequest::IsSimple().

if (request.fHelp || request.params.size() != 1)
throw std::runtime_error(
"locktransaction \"transactionid\"\n"
"\nSets InstantSend lock on given transaction on the receiver side\n"

This comment has been minimized.

@UdjinM6

UdjinM6 Jul 16, 2018

I don't see how trying to lock txes on the receiver side only can be useful tbh, you need the whole network to lock tx to make sure no double-spent tx will ever be mined. This means that you need masternodes to vote on this tx and every other node (or at least vast majority of them) should get these votes and lock tx too. I don't think you can do this via rpc, because once tx is relayed you can't re-relay ix for it and force masternodes to vote. This rpc won't do anything imo and should be removed.

/// For how long we are going to accept votes/locks
/// after we saw the first one for a specific transaction
static const int INSTANTSEND_LOCK_TIMEOUT_SECONDS = 15;
/// For how long we are going to keep invalid votes and votes for failed lock attempts,
/// must be greater than INSTANTSEND_LOCK_TIMEOUT_SECONDS
static const int INSTANTSEND_FAILED_TIMEOUT_SECONDS = 60;
/// If transaction has less or equal inputs than MAX_INPUTS_FOR_AUTO_IX,
/// it will be automatically locked
static const int MAX_INPUTS_FOR_AUTO_IX = 4;

This comment has been minimized.

@UdjinM6

UdjinM6 Jul 16, 2018

This should probably be a private member of CTxLockRequest.

static const int MAX_INPUTS_FOR_AUTO_IX = 4;
/// Automatic locks of "simple" transactions are only allowed
/// when mempool usage is lower than this threshold
static const double AUTO_IX_MEMPOOL_THRESHOLD = 0.1;

This comment has been minimized.

@UdjinM6

UdjinM6 Jul 16, 2018

This should probably be a private member of CInstantSend.

else
{
if(CInstantSend::IsTrxSimple(tx) && CInstantSend::CanAutoLock()) {
if(!instantsend.ProcessTxLockRequest(tx, connman))

This comment has been minimized.

@UdjinM6

UdjinM6 Jul 17, 2018

Thinking about this piece of code a bit more and... it probably won't do anything actually. Instead, I think you should process ix and simple txes identically. One way would be to change (strCommand == NetMsgType::TXLOCKREQUEST) conditions into (strCommand == NetMsgType::TXLOCKREQUEST || (strCommand == NetMsgType::TX && CInstantSend::IsTrxSimple(tx) && CInstantSend::CanAutoLock())) here (lines 2001+), a bit above (lines 1949+) and for rejected txes down below (on lines 2131+). This would also require smth like txLockRequest.tx = ptx; on lines 1931+.

However, I'm not 100% sure if these changes would be enough either. We should probably adjust p2p-instantsend.py to include some tests for simple txes (success for simple txes, failure for non-simple regular txes) to be a bit more confident that changes in this PR actually work.

This comment has been minimized.

@gladcow

gladcow Jul 18, 2018

Collaborator

Agree, adding tests now.

@gladcow gladcow changed the title from Automatic InstantSend locks for "simple" transactions to [WIP] Automatic InstantSend locks for "simple" transactions Jul 18, 2018

@sidhujag

This comment has been minimized.

sidhujag commented Jul 31, 2018

If this means what I think it means it would require 5 or 6 signature checks per transaction (this is partly why it should have higher fees). This will reduce the overall throughput that each mempool node is capable of processing. I would recommend IS or "locking" be opt-in with the higher fee and don't do it under the hood for them, downstream this breaks some other implementation specific stuff that relies on stuff not being locked.

@gladcow

This comment has been minimized.

Collaborator

gladcow commented Jul 31, 2018

@sidhujag , can you give more details on the stuff that can be broken, please?

@gladcow gladcow force-pushed the gladcow:recixlock branch from 1ea841e to 0bc8f8d Aug 6, 2018

@gladcow

This comment has been minimized.

Collaborator

gladcow commented Aug 7, 2018

I've added separate tests for this feature in p2p-autoinstantsend.py. It reuses some code from p2p-instantsend.py and it is correct to move this common code in some base class, but I think it is better to do this in separate PR to not blur this PR goal.

About the identical processing for simple txes and instantsend txes in net_processing.cpp: such code requires several small hacks (for example we should initialize CTxLockRequest with simple tx pointer without deserializing it from stream), it can cause errors in the future after possible serialization format changes for example. Current code is working (as we have checked with regtest) and is more compact, so I have decided to stick with it. @UdjinM6, tell me, please, if you disagree.

@gladcow gladcow changed the title from [WIP] Automatic InstantSend locks for "simple" transactions to Automatic InstantSend locks for "simple" transactions Aug 7, 2018

@UdjinM6

This comment has been minimized.

UdjinM6 commented Aug 9, 2018

I'm probably missing smth but p2p-autoinstantsend.py fails for me locally... Could you rebase this on top of the latest develop pls?

@gladcow gladcow force-pushed the gladcow:recixlock branch from aacd3e1 to 9732fb1 Sep 24, 2018

@gladcow

This comment has been minimized.

Collaborator

gladcow commented Sep 24, 2018

Rebased and fixed, re-review, please.

@UdjinM6

This comment has been minimized.

UdjinM6 commented Sep 24, 2018

Still doesn't quite fix it in the way I suggested #2140 (comment) and I'm not sure if fixes here are enough. Wouldn't this UdjinM6@c04a973 be a better way?

@gladcow

This comment has been minimized.

Collaborator

gladcow commented Sep 24, 2018

Sorry, I've misunderstood required fixes, will add this commit now

@UdjinM6

re-utACK

but would be nice if someone else would also review the latest patch with fresh eyes @nmarley @codablock

@nmarley

utACK

@PastaPastaPasta

minor formatting, see inline

bool CInstantSend::CanAutoLock()
{
if(!isAutoLockBip9Active)
return false;

This comment has been minimized.

@PastaPastaPasta

PastaPastaPasta Sep 25, 2018

body same line or brackets

if(!isAutoLockBip9Active)
return false;
if(!sporkManager.IsSporkActive(SPORK_16_INSTANTSEND_AUTOLOCKS))
return false;

This comment has been minimized.

@PastaPastaPasta

PastaPastaPasta Sep 25, 2018

bodies same line or brackets

@@ -976,6 +988,8 @@ bool CTxLockRequest::IsValid() const
CAmount CTxLockRequest::GetMinFee() const
{
if(IsSimple())
return CAmount();

This comment has been minimized.

@PastaPastaPasta

PastaPastaPasta Sep 25, 2018

bodies same line or brackets

@codablock

utACK

@UdjinM6 UdjinM6 merged commit 5454bea into dashpay:develop Sep 26, 2018

1 of 2 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@UdjinM6

This comment has been minimized.

UdjinM6 commented Sep 26, 2018

Ooops... I missed @PastaPastaPasta comments somehow 🙈 Let's fix them in some future cleanup/refactoring PR.

@gladcow

This comment has been minimized.

Collaborator

gladcow commented Sep 26, 2018

It's my fault, I've decided to fix it later and missed the merging. I'll add this fix in next PR.

@PastaPastaPasta

This comment has been minimized.

PastaPastaPasta commented Sep 26, 2018

No worries @UdjinM6 and @gladcow

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment