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

default to using a Plain kernel with no lock_height #17

Merged
merged 1 commit into from
Mar 19, 2019

Conversation

antiochp
Copy link
Member

@antiochp antiochp commented Mar 13, 2019

We currently default to generating HeightLocked transaction kernels (locked at current block height).
This PR changes this to default to generating Plain transaction kernels.

We are exploring making the lock_height optional on kernels (it is not included in the tx kernel hash for Plain kernels).
First step toward making them truly optional is to stop using them where we don't need to.

Eventually we will be able to reduce storage and bandwidth costs (a little) by only including the lock_height on HeightLocked kernels.

This PR leverages the existing grin behavior - a Plain tx kernel default to a lock_height of 0 (as we need something to store in the u64 lock_height). This is excluded from the kernel hash for Plain kernels.

TODO -

  • Some reasonably extensive testing to make sure Plain kernels are usable across all wallet and node scenarios.

@antiochp antiochp added the enhancement New feature or request label Mar 13, 2019
@antiochp antiochp added this to the 1.1.0 milestone Mar 13, 2019
@antiochp
Copy link
Member Author

@tromp Will be pleased to know about this I suspect.

@tromp
Copy link
Contributor

tromp commented Mar 13, 2019

We do include lock_height in the kernel of HeightLocked transactions, as required to prevent malleability to different lock_heights...

And yes, I'm very happy to see txs defaulting to plain!

@antiochp
Copy link
Member Author

We do include lock_height in the kernel of HeightLocked transactions, as required to prevent malleability to different lock_heights...

Yes we do. I realized my description above was somewhat ambiguous, revised to make it clearer.

@yeastplume
Copy link
Member

👍, given 1.1.0 is going to require quite a bit of testing anyhow, do you want to merge this now? Or do you want to test separately then merge?

@antiochp
Copy link
Member Author

Let me fix up the conflicts and I'll merge this in.
I can run this locally against 1.0.x nodes and it all seems to be fine.
We should keep this change in mind though when testing 1.1.0 (in case anything unexpected crops up related to this).

when constructing txs via the wallet
@antiochp antiochp merged commit 64772e6 into mimblewimble:master Mar 19, 2019
@antiochp antiochp deleted the plain_kernels branch March 19, 2019 17:31
garyyu added a commit to garyyu/grin-wallet that referenced this pull request May 6, 2019
* fix: integration Cargo.toml

* rustfmt
yyangli referenced this pull request in mwcproject/mwc-wallet May 13, 2020
default to using a Plain kernel with no lock_height
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants