Skip to content

[Multisend] Improve multisend and UTXO split handling#1096

Closed
CryptoDev-Project wants to merge 1 commit into
PIVX-Project:masterfrom
CryptoDev-Project:multisend-multifix
Closed

[Multisend] Improve multisend and UTXO split handling#1096
CryptoDev-Project wants to merge 1 commit into
PIVX-Project:masterfrom
CryptoDev-Project:multisend-multifix

Conversation

@CryptoDev-Project
Copy link
Copy Markdown

1️⃣ "MultiSendStake Activated" and "MultiSendMasternode Not Activated" and we win masternode reward

  • wallet.cpp#L3492 is false because fMultiSendMasternodeReward is false
  • wallet.cpp#L3493 is true because we have fMultiSendStake activated, the tx to which the masternode reward output belongs is a coinstake transaction and sendMSonMNReward above is false

My testing shows that we send the masternode reward in the above scenario. We need to set sendMSOnStake only when !outpoint.IsMasternodeReward(out.tx)

2️⃣ "MultiSendStake Activated" and "MultiSendMasternode Not Activated" and we win both masternode and staking reward in same wallet

In this outlier case scenario, we'll go through the loop beginning wallet.cpp#L3485 twice - once for the stake reward and once for mn reward. Both times, sendMSOnStake will hold true. In each case, nAmount is calculated as (out.tx->GetCredit(ISMINE_SPENDABLE) - out.tx->GetDebit(ISMINE_SPENDABLE). As we own spendable coins equating to the entire block value, the first time through the loop will result in us sending the entire block reward using the stake output as an input. The second time will involve us attempting to send the entire block value a second time using the masternode reward as input. This transaction will fail as masternode reward input is insufficient when attempting to send (masternode reward + staking reward).

The fix for 1️⃣ removes the second run through the loop, but we still send the entire block value as we earned both masternode and staking reward, both now considered ISMINE_SPENDABLE from the relevant TX. During testing, even with the second loop removed, I also obeserved failed multisend transaction commits where (stake input + stake reward) < block value. We need to remove the stake reward or masternode reward values for wallets that haven't activated those.

The fix implemented for 2️⃣, which factored in the requirements for both reward categories, results in only the staking reward being sent via multisend when only MultiSendStake is activated. I then switched to having only MultiSendMasternode activated and, as required, the staking reward is deducted from the value sent. We're now accounting for when either, but not both, options are selected.

3️⃣ "MultiSendStake Activated" and "MultiSendMasternode Activated"

Having implemented the above fixes, I now activate multisend for masternodes and staking. Once again, behavior deviates from expectations. This is because we are looping through the code twice where one wins both masternode and staking reward. The masternode reward output, like the stake output before it, attempts to send the entire block reward via multisend and fails. Therefore, when we 'set which MultiSend triggered', we must set both as having being triggered if we won, and sent via multisend, the entire block reward.

4️⃣ Excessively low user setting for stake split threshold

Even after the above changes, we still face issues where (stake input + stake reward) < block value and we win both masternode and staking reward. However, this is such an outside case that I feel it's okay to let the multisend transaction fail. The code would need a rewrite to add additional inputs to the transaction and is simply not worth it in my opinion given that it's an issue that's likely to never happen, but with extremely minor consequences if it does, i.e. a multisend transaction failure.

However, even if we don't win both masternode and staking reward, we will encounter issues where stakesplitthreshold is set so low that one of the UTXO's fails to cover the amount to be sent via multisend. To resolve this issue, I have added code to override the users stakesplitthreshold with the block value if they happen to choose anything below that value.

5️⃣ Stake split decision including masternode reward in error

During testing, I also observed that wallet.cpp#L2699 uses nCredit solely to determine whether we actually need a split. However, nCredit includes the masternode reward in error. Therefore, I've deducted it from nReward for the purposes of the call to CreateTxOuts.

6️⃣ Multisend after stake resulting in UTXO split

When a stake reward results in a UTXO split, the resulting multisend transaction is attempted multiple times, once for each UTXO from the stake split. This was the original fix I provided, and must remain.

@random-zebra random-zebra added this to the 5.0.0 milestone Apr 24, 2020
@furszy furszy modified the milestones: 4.2.0, 5.0.0 Jul 1, 2020
@random-zebra random-zebra modified the milestones: 5.0.0, Future Sep 22, 2020
@random-zebra
Copy link
Copy Markdown

Multisend has been definitely removed in #2293. Closing.

@random-zebra random-zebra removed this from the Future milestone Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants