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.

Sign up for GitHub

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

Jump to bottom

Feature/slate serialization #2534

Merged
merged 17 commits into from
Feb 13, 2019
Merged

Feature/slate serialization #2534

merged 17 commits into from
Feb 13, 2019

Conversation

svechinsky
Copy link
Contributor

@svechinsky svechinsky commented Feb 5, 2019 •
edited
Loading

This change enables hex serialization for slate objects in a backwards compatible way by introducing enum based versioning for the slate.
This was the nicest way I could find/think of to do this using serde.

It will enable an upgraded client to receive grin from an old client but not the other way around (this would require passing another argument down to the send function).

Probably shouldn't be merged until after #2533 is merged, my branch is updated and some more testing is done.
If anyone wants to help with testing it would be lovely since I'm a bit swamped with outside stuff atm.
I'll try and get to it as soon as I can, have never used keybase so would especially appreciate help here.

Testing is done, ready for CR and merge.

  • File transactions
  • Http transactions
  • Keybase transactions

@yeastplume
Copy link
Member

Looks good so far! Let me know when it's ready for a full review and I'll look in more detail as well as perform some testing.

@yeastplume
Windows Compatibility Fixes #1 (mimblewimble#2535)
15c7a27 
* initial changes for windows build and unit/integration tests

* rustfmt

* wallet+store tests

* rustfmt

* fix linux daemonize

* better encapsulate file rename

* rustfmt

* remove daemonize commands

* rustfmt

* remove server start/stop commands

* add ability to drop pmmr backend files explicitly for txhashset unzip

* rustfmt

* fix pmmr tests

* rustfmt
@yeastplume
Windows TUI Fix (mimblewimble#2555)
200f87b 
* switch pancurses backend to win32

* revert changes to restore test
@svechinsky
Copy link
Contributor Author

I'm really getting quite swamped with outside work, if anyone can help a bit with testing, especially the keybase part would be a huge help

@mcdallas
Copy link
Contributor

mcdallas commented Feb 10, 2019 •
edited
Loading

EDIT: nvm you already did

Tested send via keybase to an old node and it doesnt work. The old node expects the old slate format so how can this work in a backward compatible way ?

@mcdallas
Copy link
Contributor

Sending from old to new doesn't work either, the new node gets the slate, responds but the old node fails to deserialize and doesnt finalize the transaction

@svechinsky
Copy link
Contributor Author

svechinsky commented Feb 10, 2019 via email

@mcdallas
Copy link
Contributor

@svechinsky I sent a fix to your branch.

This PR breaks receiving grins for anyone who doesn't upgrade. Should we save it for the hard fork ?

@svechinsky
Copy link
Contributor Author

I think the plan is to enable upgraded nodes to receive grins from non-upgraded nodes.
If someone wants to receive grins they can upgrade.

Thanks for the keybase fix!!!

@svechinsky
Copy link
Contributor Author

Ok @yeastplume, tested for http should be ready now

garyyu and others added 2 commits February 12, 2019 13:39
@garyyu @yeastplume
add repost method into wallet owner api (mimblewimble#2553)
5987bcd 
* add repost method into wallet owner api

* rustfmt
@i1skn @yeastplume
Add ability to compare selection strategies (mimblewimble#2516)
1d0c04c 
Before tx creation user can estimate fee and locked amount
with different selection strategies by providing `-e` flag for
`wallet send` command.
@yeastplume
Copy link
Member

Sorry, merge conflict introduced.

I think this looks great, but I'm a little bit unsure as to how we should go about merging this. (I want to get this in one way or another so I can start splitting the wallet out).

Either we:

  • Merge now and tell anyone running wallets < 1.0.2 to upgrade
  • Merge into 1.1 branch

I think given this is non-consensus breaking and there will be some upgrade pain anyhow due to the upcoming wallet split, I'm not against merging this into master as is. Thoughts @antiochp @ignopeverell ?

@ignopeverell ignopeverell added this to the 1.1.0 milestone Feb 12, 2019
@ignopeverell
Copy link
Contributor

To keep sane versioning I think we should introduce that with 1.1.0 and then point out in release notes and various configuration that wallet and server should be upgraded at the same time.

@ignopeverell ignopeverell changed the base branch from master to milestone/1.1.0 February 12, 2019 23:02
@ignopeverell
Copy link
Contributor

Targeting 1.1.0 introduced a conflict, sorry about that. Also not really a WIP anymore, right?

@svechinsky svechinsky changed the title [WIP] Feature/slate serialization Feature/slate serialization Feb 13, 2019
@svechinsky
Copy link
Contributor Author

Fixed both :)

@garyyu
Copy link
Contributor

garyyu commented Feb 13, 2019

@svechinsky
Did you use rebase on your branch? it has imported several other PRs into this PR.
Normally I use merge instead of rebase, to avoid this problem.

And unfortunately, each time when I faced this problem, I have to create another new branch (from your target branch, in your case, it could be milestone/1.1.0), and cherry-pick the real own commits into it and submit a new PR. Perhaps others can propose a better solution to fix this.

@svechinsky
Copy link
Contributor Author

I fetched the new branch (milestone/1.1.0) and merged (with merge not rebase) it into mine.
I'm not sure what went wrong.

@svechinsky
Copy link
Contributor Author

Oh I understand what happened. It also wants to merge all the commits since 1.1.0 was seperated from master.
I'm not sure if this is such a bad thing but if it is then I'll cherry pick as @garyyu suggested a bit later today.

@yeastplume
Copy link
Member

I don't think this is a bad thing, the other commits that have been picked up are stuff that's needed anyhow, windows support and http repost.

I'm going to merge this as is then sort out any issues manually.

@yeastplume yeastplume merged commit ee4eed7 into mimblewimble:milestone/1.1.0 Feb 13, 2019
@garyyu garyyu mentioned this pull request Mar 7, 2019
Closed
@antiochp antiochp added the release notes To be included in release notes (of relevant milestone). label Jun 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement release notes To be included in release notes (of relevant milestone).
Projects
None yet
Milestone
1.1.0
Development

Successfully merging this pull request may close these issues.
8 participants
@svechinsky @yeastplume @mcdallas @ignopeverell @garyyu @antiochp @rentenmark @i1skn