-
Notifications
You must be signed in to change notification settings - Fork 123
sweepbatcher: allow swap_hash to be non-unique #906
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
Conversation
332e2bc to
86c9884
Compare
689061d to
0ae934b
Compare
bhandras
left a comment
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.
LGTM, very nice! 🥇
|
|
||
| // primarySweepID is the swap hash of the primary sweep in the batch. | ||
| primarySweepID lntypes.Hash | ||
| primarySweepID wire.OutPoint |
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.
nit: update godoc.
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.
Fixed. Thanks!
|
|
||
| // sweepExists returns true if the batch contains the sweep with the given hash. | ||
| func (b *batch) sweepExists(hash lntypes.Hash) bool { | ||
| func (b *batch) sweepExists(outpoint wire.OutPoint) bool { |
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.
nit: update godoc.
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.
Fixed.
| ); | ||
|
|
||
| -- Copy all the data from sweeps to sweeps2. | ||
| WITH RECURSIVE seq(i) AS ( |
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 I understand this query now, but could you add a short explanation of what we do so it looks less intimidating?
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 added explanations of sqlite query to the migration file and adjustments for postgres to loopdb/postgres.go.
0ae934b to
cc67a11
Compare
hieblmi
left a comment
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.
tAck ✅
1.) created fast-swaps on the master branch
2.) ran the migration, outpoints look good.
3.) submitted a few swaps to ensure they are stored in the new format.
| @@ -0,0 +1,3 @@ | |||
| -- We kept old table as sweeps_old. Use it. | |||
| ALTER TABLE sweeps RENAME TO sweeps_new; | |||
| ALTER TABLE sweeps_old RENAME TO sweeps; | |||
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.
don't we have to drop sweeps_new to reverse the creation?
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 is dangerous, since it would be irrevocable. If some sweeps were added after the migration, they would be lost. Ideally we should write full SQL query which would split outpoint back to outpoint_txid and outpoint_index. Then we don't need sweeps_old and could just remove an unneeded table.
SwapHash used to be a key in the sweeps table. Now the key is outpoint which replaces columns outpoint_txid and outpoint_index. In-memory structures and unit tests were also updated to use outpoint as key. Outpoint is truly unique.
cc67a11 to
c80ebb9
Compare
SwapHash used to be a key in the sweeps table. Now the key is
outpointwhich replaces columnsoutpoint_txidandoutpoint_index. In-memory structures and unit tests were also updated to use outpoint as key. Outpoint is truly unique.DB migration: this PR involves a DB migration. Table
sweepsis recreated to make columnswap_hashnon-unique and to create columnoutpointfrom columnsoutpoint_txidandoutpoint_index. Columnoutpoint_txidis byte-reversed and converted to HEX. I tested the migration manually on both postgres and sqlite.Pull Request Checklist
release_notes.mdif your PR contains major features, breaking changes or bugfixes