-
Notifications
You must be signed in to change notification settings - Fork 232
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
Add support for auto-compound in kaspawallet send
#1951
Add support for auto-compound in kaspawallet send
#1951
Conversation
…t that it can be used in kaspawallet
b97f1de
to
ee60333
Compare
@@ -95,3 +102,32 @@ func (s *server) selectUTXOs(spendAmount uint64, feePerInput uint64) ( | |||
|
|||
return selectedUTXOs, totalValue - totalSpend, nil | |||
} | |||
|
|||
func (s *server) oneMoreUTXOForMergeTransaction(alreadySelectedUTXOs []*libkaspawallet.UTXO, requiredAmount uint64) (*libkaspawallet.UTXO, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this method to split_transaction.go, close to where it's used
return serialization.DeserializePartiallySignedTransaction(mergeTransactionBytes) | ||
} | ||
|
||
func (s *server) maybeSplitTransaction(transaction *serialization.PartiallySignedTransaction, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach of splitting a transaction doesn't sit well with me. I couldn't figure out a concrete example, but I'm certain there could be edge-cases where this could produce split transactions over the mass limit. What more, we never even check that the transactions created by this method are valid
Instead, I propose a far less efficient, but safe approach:
- Start a new transaction
- Add inputs and for each one check if we're over the limit
- Once we're just under the limit, finish this transaction and move on to the next
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel ya, and also @someone235 has defined said edge case, but I have a feeling what you suggest is an overkill.
It also will probably be quite annoying, clocking at O(n^2) with a possibly high n, and high constants (with cloning of the tx, etc.), in an operation that is supposed to be instantaneous.
I think the way I did it now is much more foolproof.
Please take a careful look in and around splitAndInputPerSplitCounts. (Also suggestions for a better name welcome).
sentValue := originalTransaction.Tx.Outputs[0].Value | ||
utxos := make([]*libkaspawallet.UTXO, len(splitTransactions)) | ||
for i, splitTransaction := range splitTransactions { | ||
output := splitTransaction.Tx.Outputs[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't appear to be merging the change. Is it not something we want to do at this stage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sum of the outputs of the splits is equal to the total of their inputs. This includes the change as well.
Change is re-calculated again in mergeTransaction:
if totalValue > sentValue {
payments = append(payments, &libkaspawallet.Payment{
Address: changeAddress,
Amount: totalValue - sentValue,
})
(Note the change is going to be a bit lower, because there's fee for the mergeTransaction)
Codecov Report
@@ Coverage Diff @@
## dev #1951 +/- ##
==========================================
- Coverage 57.86% 57.53% -0.33%
==========================================
Files 676 679 +3
Lines 32172 32403 +231
==========================================
+ Hits 18616 18644 +28
- Misses 10919 11115 +196
- Partials 2637 2644 +7
Continue to review full report at Codecov.
|
return err | ||
} | ||
fmt.Println("Transaction was sent successfully") | ||
fmt.Printf("Transaction ID: \t%s\n", response.TxID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please somehow combine the above two logs so that the user doesn't see multiple "Transaction was sent successfully" messages on separate lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did something a little bit different, see what I did and lmk if you no like
} | ||
} | ||
if totalValueAdded < requiredAmount { | ||
return nil, 0, errors.Errorf("Insufficient funds for merge transaction") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want to deny a user from sending funds just because they don't have enough money for a merge transaction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the alternative?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not having funds for merge transaction is more-or-less the same as not having enough fees, just send a little bit less and you'll be fine.
|
||
if totalValue < sentValue { | ||
// sometimes the fees from compound transactions make the total output higher than what's available from selected | ||
// utxos, in such cases - find one more UTXO and use it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update this comment
return splitTransactions, nil | ||
} | ||
|
||
func (s *server) splitAndInputPerSplitCounts(transaction *serialization.PartiallySignedTransaction, transactionMass uint64) ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the sake of argument, let's assume our transaction contains:
- 9 pay to public key inputs, each one has mass 100
- 1 multisig input with mass 900
- Mass limit of 1000
- A transaction without any inputs has mass 50
I believe this method would produce:
- massWithoutInputs = 50
- massForInputs = 1800
- inputCount = 10
- massPerInput = 180
- inputPerSplitCount = 5
- splitCount = 2
The splitCount is correct, but if we try to have inputPerSplitCount of 5, we'd get:
- One transaction with mass 550
- Another transaction with mass 1350
1350 is over the mass limit of 1000
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We assume that the transaction was created by kaspawallet, thus it wouldn't have inputs with different masses.
Therefore your scenario is impossible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add a comment that explains it. It could also be nice to run some sanity test to check if the assumptions are correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a comment.
Adding a sanity test means either refactoring the CalculateMass code to have CalculateMassForInput, or using some not-entirely-accurate proxy such as input size, and I'd rather not do either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can check if all inputs are multisig with the same number of required signatures, or all of them are p2pk
} | ||
} | ||
if totalValueAdded < requiredAmount { | ||
return nil, 0, errors.Errorf("Insufficient funds for merge transaction") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the alternative?
return splitTransactions, nil | ||
} | ||
|
||
func (s *server) splitAndInputPerSplitCounts(transaction *serialization.PartiallySignedTransaction, transactionMass uint64) ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add a comment that explains it. It could also be nice to run some sanity test to check if the assumptions are correct.
massPerInput++ | ||
} | ||
|
||
inputPerSplitCount = int((mempool.MaximumStandardTransactionMass - massWithoutInputs) / massPerInput) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe rename to inputsPerSplitCount?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each split tx has only one output right? So wouldn't it make more sense to reduce the mass of one output, instead of all outputs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each split tx has only one output right? So wouldn't it make more sense to reduce the mass of one output, instead of all outputs?
When there's no change - there's only 1 output in the original transaction. You are right that in most cases (a.k.a. when there is change) massForInputs wouldn't be a tight bound, but to make it tight I'd need some way to calculate CalculateMassForOutput, and this is something we don't have rn (same problem as with CalculateMassForInput)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the outputs of the original transaction as an upper bound is confusing from the reader POV (why use such outputs that are unrelated to the actual split transaction), and the calculation might go wrong: e.g. P2SH is 34 bytes long and P2PK is 33 bytes long, so when you're using a multisig wallet and wanna pay to P2PK, and say your original transaction had one output, the output of the split transaction will use a P2SH address, which requires more mass, so your split tx can go above limit.
So if you don't want to export another function for calculating output mass, can't you just make a stub output that will use some change address and then calculate the output mass in the same way you calculated massForInputs
?
This PR fixes a big problem in kaspawallet - when the number of inputs is greater then one can fit into a single transaction (such as often happens when a big miner wants to send a lot of kaspa) - the transaction generated is rejected by the node.
This PR splits such transaction into multiple smaller transactions, sending the funds into a change address, then adds another, merge transaction, which would send the funds to their final destination.
Note: there are a quite a few redundant serialize/deserialize of PartiallySignedTransaction(s)
This is there because libkaspawallet exposes methods that work with byte-slices rather then PartiallySignedTransaction(s)
I plan to refactor libkaspawallet to expose only methods for deserialized data in an upcoming PR.