Skip to content
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

MTX - Resolve coins from coinview as well during coinselection. #901

Merged
merged 1 commit into from
Aug 27, 2024

Conversation

nodech
Copy link
Contributor

@nodech nodech commented Aug 26, 2024

This is another simpler alternative to the: #666

It still gets rid of the inputs for the coinselection, but will make sure to check if coins are in coinview first and Coins list second. This also fixes another issue, where existing inputs in the tx could get in conflict with passed inputs in the options.

Closes: #666
Closes: #639

@nodech nodech added primitives part of the codebase patch Release version labels Aug 26, 2024
@nodech nodech requested a review from Anunayj August 26, 2024 13:19
@nodech nodech mentioned this pull request Aug 26, 2024
@coveralls
Copy link

Coverage Status

coverage: 69.805% (+0.003%) from 69.802%
when pulling 15e9b8f on nodech:mtx-fund-2
into 9ddb69e on handshake-org:master.

@Anunayj
Copy link
Contributor

Anunayj commented Aug 27, 2024

This is so much cleaner and I think it's probably ideal to keep it as simple as possible. There's just a few things to keep in mind as of right now:

  1. fund() will steal clear witness items, and those will need to be added back.
  2. It will also clear nSequence for all inputs! I am unsure how big of a deal this is, but please beware when using setLocktime or using Relative timelocks before calling fund!

Copy link
Contributor

@Anunayj Anunayj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation is significantly cleaner than the previous, but we would still have be cautious about potential interactions between nLocktime, nSequence, and fund() in the future. Specifically, NEVER call fund after setting nLocktime or nSequence.

  • nLocktime is likely to be less of an issue, as it will be active as long as one of the inputs is < 0xFFFFFFFF, and It throws a a exception when called with no inputs. However there can still be issues when the other inputs are ANYONECANPAY and Locktime being active was extremely important.

  • nSequence being reset could cause a more significant problem if someone is unaware of this and was expecting Relative Locktimes to be active.

We might want to consider a future revision to address these concerns, as they aren't necessarily within the scope of this PR. There also needs to be a discussion on what behavior should be expected from addCoin() when nLocktime is set.

@nodech nodech merged commit e88734f into handshake-org:master Aug 27, 2024
6 checks passed
@nodech nodech deleted the mtx-fund-2 branch August 27, 2024 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Release version primitives part of the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants