Limit "send all" batches to policy weight limit and fix estimated weight limit calculation #708
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- apply witness discount to multisig spends - fix "fallback" 2-of-3 script hash size value - account for large multisig scripts in script size varInt size
This will prevent the "Could not resolve preferred inputs" error that is thrown when a user sends a single reveal followed by a revealAll
|
Related upstream work: bcoin-org/bcoin#858 |
pinheadmz
commented
Apr 8, 2022
| * Unlike Bitcoin, our signatures are always 65 bytes. | ||
| * However, we still assume that the witness varInt size | ||
| * is only one byte. In short, this estimate may be off | ||
| * by 1 (at most) but only if a witness has > 253 items. |
There was a problem hiding this comment.
...actually if the witness stack is too big, this varInt will be 3 bytes so off by 2 at most.
Anunayj
added
bug
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes a few related wallet bugs including a new one I hadn't noticed before.
revealAllorredeemAllwith too many BIDs (like around 600) the resulting transaction will be too big to send, violating the relay policy weight limit. This is a big issue because loss of funds can occur if reveals are not sent in time. The solution is to recalculate the estimated weight of the batch as we are building it, and break before the result is too heavy.TODO:
sendRevealandsendRedeemas well? (They also produce batches)Once the batch has hit the limit and stops adding outputs, there may be a number of BIDs still unrevealed or unredeemed in the wallet. We want the user to be able to send those in another batch right away. However, they would encounter the
"Could not resolve preferred inputs"error because the second batch would try to double-spend outputs in the first batch. This error is common when users reveal a single name and then callrevealAllbefore that first single reveal has confirmed. The solution is pretty simple: when we add reveals to a batch, we skip over outputs that are marked as spent already in txdb. This required the addition of a new wallet functiongetUnspentCoin()While working with the TX size estimator to detect policy weight limit breaches, I noticed the MTX size estimator had unfinished bugs left over from its port from Bitcoin. In particular, we were not applying the witness discount to the scripts and signatures in multisig transactions. If you try to send a multisig transaction right now with master branch, you'll pay almost 100% extra fee because the estimator incorrectly computes a way bigger virtual size. The solution was to tweak the size estimator functions both in
wallet.jsandmtx.js. Also, since HNS signatures are fixed size (always 65 bytes as opposed to Bitcoin's 70-73 bytes) the estimator functions can be dialed in to almost always guaranteed accuracy. Tested.