-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
channeldb: refactor UpdateInvoice
to make it simpler to create SQL specific implementation
#8100
channeldb: refactor UpdateInvoice
to make it simpler to create SQL specific implementation
#8100
Conversation
e3dd68a
to
68d34a3
Compare
68d34a3
to
f91c3fd
Compare
UpdateInvoice
to make it simpler to create SQL specific implementationUpdateInvoice
to make it simpler to create SQL specific implementation
5a99f61
to
a535c6e
Compare
I'm not 100% about this approach. Another would be to more explicitly flag changes in the update contexts as currently we pass around the changed invoice and assume that the underlying updater implementation will know what to do. For the key/value case it's simple as we just serialize and store. |
25bd7c9
to
94775ea
Compare
Just about to dive in my prior to this my mental model was something like:
|
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.
Great initial stab at this very important refactor!
Dropped some ideas of an alternative where we center the abstractions around encapsulated invoice updates, with the db transaction abstracted over using something similar to your current InvoiceUpdater
. Lemmie know what you think, I can work on some PoC API code if we link this direction better. Goal here is to have most of the biz logic in the invoice registry, a middleware that takes the abstract updates to map into concrete db operations, then the lowest level that actually applies the updates.
For that middle layer, we'd have a unified interface that the invoice registry would use to queue up the updates. Then for KV land and SQL land we have distinct implementations. Then future backends implement the mapping from the middleware interface to how the operations would be expressed using that concrete backend.
channeldb/invoices.go
Outdated
return invoice, nil | ||
} | ||
|
||
func (d *DB) addHTLCsStoreUpdate(invoices, settleIndex, setIDIndex kvdb.RwBucket, |
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.
Cool, I think this is inline with what I had in my pending mental model. Can be abstracted one layer further as well, since rn it gives it raw access to the various buckets. I'm thinking we decouple the description of the update, from the application itself. Under that mental model, this function is the application, with the set of arguments making up part of the description. The gap here then is taking a method call and mapping it to some description, to be applied atomically at a later step.
One way to sort feel out these set of changes father would be to remove the Invoice
argument from most/all of the update methods. Then the set of methods would be operating on an abstract InvoiceUpdate
interface, which collects the updates, then applies them. For KV land application can just be mutate the invoice then sync to disk.
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 was also thinking this would be one layer higher. So most of this biz logic is now in the invoice
package, then the idealized channeldb
package is just doing the update (tho it is the case that rn we have a lot of biz logic in chanenldb
, and part of the original UpdateInvoice
call back was meant to help us start to extract that logic.
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.
Please note that the first few commits are just preliminary refactors in order to introduce the InvoiceUpdater
and also to decouple in-memory invoice updates and DB updates. I should have flagged this in the PR description, but my general idea to do the least intrusive changes was to:
- separate DB updates to their own functions
- introduce a common interface and KV implementation for the updates
- move all the update invoice logic to the
invoices
package - move all non-kv related tests to the
invoices
package too
This way we can implement an updater that's compatible with the existing implementation and works on SQL. The challenge here imho is the right level of abstraction as well as being careful with any changes to the Invoice
or the in-memory update logic itself since our test coverage is somewhat limited. My choice here was to first make things work with a SQL updater in a naive way, and once we can deprecate the KV invoice DB do a deeper refactor to have a more SQL friendly implementation.
I'm looking into your idea of a more streamlined approach as I agree that having helpful types and more interfaces can generally help.
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.
In the mean time I'm working on the separate branch in parallel to actually implement the missing bits for the SQL port, so this PR sometimes changes as I discover simpler or more elegant ways to separate things.
channeldb/invoices.go
Outdated
if err != nil { | ||
return nil, err | ||
} | ||
|
||
return invoice, nil | ||
} | ||
|
||
func (d *DB) cancelInvoiceStoreUpdate(invoices kvdb.RwBucket, invoiceNum []byte, | ||
invoice *invpkg.Invoice) error { | ||
|
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.
Not much distance between this and the old call site?
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 really just a boring preliminary step to separate in-memory vs storage stuff.
channeldb/invoices.go
Outdated
} | ||
|
||
switch update.UpdateType { | ||
case invpkg.CancelHTLCsUpdate: | ||
return d.cancelHTLCs(invoices, invoiceNum, &invoice, update) | ||
return d.cancelHTLCs(invoices, invoiceNum, invoice, update) |
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 the enum above is part of the way there, but rather then switch on the enum, we make a new InvoiceUpdater
from the invoice itself (the KV impl maybe still copies like above), then we do ApplyUpdate
on each of the abstract update descs, which are the items in the callback today), then finally commit. The internal impl then does a type switch or w/e on each of the updates to figure out how to apply it.
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 choose to go with a similar although quite different approach. The update desc and in-memory update stays the same to avoid very intrusive code changes and instead the in-memory stuff is restructured a bit so that the DB stuff can be more fine-grained (which is ideal for the SQL stuff). PTAL all the commits, especially the one adding the InvoiceUpdater
which will clear up why these changes are made.
channeldb/invoices.go
Outdated
// invoice in the database. The methods of this interface are called when the | ||
// in-memory update of an invoice is complete, and the database needs to be | ||
// updated. | ||
type InvoiceUpdater interface { |
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.
great minds think alike ;)
channeldb/invoices.go
Outdated
type InvoiceUpdater interface { | ||
// StoreCancelHtlcsUpdate updates the invoice in the database after | ||
// cancelling a set of HTLCs. | ||
StoreCancelHtlcsUpdate(InvoiceUpdaterContext) error |
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.
What about a slightly diff model where:
- We have two main methods
ApplyUpdate
, thenCommit
. ApplyUpdate
takes something similar toInvoiceUpdaterContext
, but specific for each type of update. This is a new interface,InvoiceUpdate
which is then destructed within theApplyUpdate
method to figure. out how to apply it.Commit
makes the db transaction, and tries to commit it all. If this fails, everything else above fails as well.
Eg we can take StoreAddHtlcsUpdate
and make a new struct of NewHtlcUpdate
that just has a field of the HTLCs to update, and w/e other information. This gets passed into ApplyUpdate
that figures out how to stage it, if the state is invalid, etc. Ideally we can have the concrete implementation of the *Update
structs handle the validation. Eg: you can't add more HTLCs if the state of the invoice is already cancelled, etc. Then we have all these smaller update modules, that can be tested in isolation.
The main InvoiceUpdate
interface can be pretty slim. Mainly just to allow polymorphism to simplify the call site at the invoice registry: figure out all updates, make a new updater, apply then all, then commit. So maybe:
type InvoiceUpdate interface {
InvoiceRef() invoice.InvoiceRef
Type() invoice.UpdateType
ValidateUpdate(oldInvoice *Invoice) error
}
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.
Thank you for the great suggestion!
IIUC what you describe is essentially the same model that's present in the PR + the existing in-memory update methods, with the added benefit of more streamlined approach through types and interfaces. So in this proposed approach the "ApplyUpdate" part is where we apply the update to the copied invoice in memory (with the existing legacy methods) and "Commit" is the InvoiceUpdater
interface without the nice abstraction since we pass around the mutated+new state in the context.
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 see a deeper refactor where the invoices.Invoice
type could only be mutated through specific methods and the update itself would just call into those methods. That way the mutated state could just be queue'd up and committed to the database later. I'm open to such big change too, but its also not without risk as it implies changes to existing tests etc.
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 your suggestion is very similar to what @ziggie1984 expressed below, but I also think that doing a full-fledged "deep" refactor would be a lot more work that I'd like to first justify vs a simpler approach presented here. I think a more gradual multi PR refactor could be built as follow up once the e2e functionality is rock solid. The approach presented in this PR has a working SQL implementation too which we could use (once it's solidified) as a v1. wdyt?
// invoice in the database. The methods of this interface are called when the | ||
// in-memory update of an invoice is complete, and the database needs to be | ||
// updated. | ||
type InvoiceUpdater interface { |
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.
+1 for having most of the logic here in the invoices
package
Then we have another layer between this and the DB, that's responsible for mapping the invoice updates into concrete DB operations.
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.
My main goal for this was to completely decouple invoice tests from the channeldb
package such that we're easily able to test against other InvoiceDB
implementations such as the SQL one.
913f01d
to
bd604c9
Compare
Please take reviews on-hold while I'm working on a larger refactor to accommodate suggestions by Laolu. Will move out of draft when ready for another look. |
bd604c9
to
5e16ec8
Compare
Really important and also difficult PR you are working on, respect 🤝 ! I like your approach and how you separated a lot of data from the db to the invoice package. What caught my eye in this new approach is, that we have a lot of functions not really used in the I like the proposal of laolu abstracting everything even further. I think this approach would keep most of your code but separate them even more, wdyt ? For example: In the
This DBUpdater return your UpdateInvoicer interface:
The UpdateInvoice provides all the information which update we want to supply and what I think is very important to extract all the Validation logic which lives currently in more than one place just in one function ?
Then for a particular invoice update, let's say for the AddHTLC case currently living in
Then all the stuff how the Update is staged would be separate for both backends, so we would get rid of all the function currently not used by the KV-World ? I have to point out your solution does also the job as well and I am not sure how much redundant code we would generate in the both @Roasbeef you were talking about a middle-layer would be interesting how you would envision this interface to look like ? |
259b2ae
to
fbd6170
Compare
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.
Thank you all for the reviews! 🙏 Let me just quickly also ask that if you would please ignore commenting on bfcddd9 as that commit just moves code around and I'd like to not add anything there to simplify the rebases a bit (as git is unable to properly track changes).
channeldb/invoices.go
Outdated
@@ -2688,10 +2709,10 @@ func updateHtlc(resolveTime time.Time, htlc *invpkg.InvoiceHTLC, | |||
return trySettle(false) | |||
|
|||
case invpkg.ContractOpen: |
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.
IIUC if the invoice is AMP then by accepting an HTLC we'd still have the state as ContractOpen
. To validate I've commented this case and ran the unit tests and it failed exactly when we were trying to accept an AMP HTLC on a newly added invoice.
|
||
// invoice is the invoice that we're updating. As a side effect of the | ||
// update this invoice will be mutated. | ||
invoice *invpkg.Invoice |
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.
Well we're actually changing the invoice, because the update to the fetched invoice is done in-memory too. The other option would be to do the update purely in the db on the copied invoice and then fetch it again?
return invoice, nil | ||
} | ||
|
||
switch update.UpdateType { |
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 agree there's much room for more refactoring. Maybe we can do deeper cleanups in a follow up once the SQL version is in. WDYT?
invoiceState ContractState, setID *[32]byte) ( | ||
bool, HtlcState, error) { | ||
|
||
trySettle := func(persist bool) (bool, HtlcState, error) { |
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.
Since this commit is just moving code around, perhaps best to recreate these comments on other commits as every time I rebase I have to manually recreate this commit. Also maybe just to reduce the scope of the PR, perhaps we could do these refactors in a follow up. WDYT?
IMO this is good to go once the questions and the potential nil panic are resolved. It'd be great to have this merged so we can focus on its following PR. Again great work on the separation of handling ephemeral and permanent states of invoices👏 Just to be clear, the followings are non-blocking, as refactoring can be an endless endeavor,
|
channeldb/invoices.go
Outdated
|
||
ampState.InvoiceKeys[circuitKey] = struct{}{} | ||
|
||
// Due to the way maps work, we need to read out the value, update it, |
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.
Think the comment is a bit outdated, also inaccurate since if we store a pointer then we won't have this issue. Maybe remove it?
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.
done
channeldb/invoices.go
Outdated
continue | ||
// Verify that we don't get an action for htlcs that are not | ||
// present on the invoice. | ||
if !exists { |
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.
wanna note that this does change the behavior a bit - previously we'd cancel the intersection of the two sets then error out, now we'd error out before canceling the rest, I guess this would lead to some htlcs hanging?
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.
Yeah I agree this looks suspicious but the overall behavior won't change because any error during the update will abort the transaction.
channeldb/invoices.go
Outdated
htlc.State = htlcState | ||
htlc.ResolveTime = resolveTime | ||
newState := htlc.State | ||
if persist && settled { |
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.
persist
can be removed now since we are not mutating here?
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.
Indeed, done.
channeldb/invoices.go
Outdated
// validate the state transition if we're cancelling the invoice. | ||
func getUpdatedInvoiceState(invoice *invpkg.Invoice, hash *lntypes.Hash, | ||
update invpkg.InvoiceStateUpdateDesc) (*invpkg.ContractState, | ||
*lntypes.Preimage, error) { |
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.
still getting through, but this method behaves more like a validator than a getter - we pass in an update
, and return update.NewState
and update.Preimage
. I think at least we can stop returning preimage here and let the caller access it via update.Preimage
. Or better, do something like canCancelSingleHtlc
and only return an error.
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.
Good idea! Done.
valueLen := copy(indexKey[:], k.invoiceNum) | ||
|
||
if setID != nil { | ||
valueLen += copy(indexKey[valueLen:], setID[:]) |
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 don't think the copied date is used here so valueLen
can be removed.
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.
Let's keep it for readability.
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.
by readability you mean to keep it so it's more like a pure code move than any functionality change?
invoices/update_invoice.go
Outdated
AcceptTime: updateTime, | ||
State: HtlcStateAccepted, | ||
CustomRecords: htlcUpdate.CustomRecords, | ||
AMP: htlcUpdate.AMP.Copy(), |
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 may panic?
// InvoiceDB instance. The purpose of this test is to be able to run all tests | ||
// with a custom DB instance, so that we can test the same logic with different | ||
// DB implementations. | ||
func TestInvoiceRegistry(t *testing.T) { |
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'm not in favor of this testing setup - why are we testing the database implementation details in invoices
, while each implementation should have their own dedicated unit tests? It's wonderful that we finally have the InvoiceUpdater
interface, and our tests should stop at the interface, with each interface method being mocked using its defined rules, and provide more robust unit tests at the implementation side. Meanwhile it's more suitable to use integration tests to check different db behaviors.
Thanks for the review, will address comments asap!
Something that might not be super obvious that there're plenty of tests that test the integration of the updater (
Fortunately |
487ad5d
to
e3446dc
Compare
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.
Thanks for the super quick review 🏎️ @yyforyongyu 🙏
channeldb/invoices.go
Outdated
|
||
ampState.InvoiceKeys[circuitKey] = struct{}{} | ||
|
||
// Due to the way maps work, we need to read out the value, update it, |
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.
done
channeldb/invoices.go
Outdated
continue | ||
// Verify that we don't get an action for htlcs that are not | ||
// present on the invoice. | ||
if !exists { |
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.
Yeah I agree this looks suspicious but the overall behavior won't change because any error during the update will abort the transaction.
channeldb/invoices.go
Outdated
@@ -2293,14 +2302,20 @@ func (d *DB) settleHodlInvoice(invoices, settleIndex kvdb.RwBucket, | |||
// TODO(positiveblue): this logic can be further simplified. | |||
var amtPaid lnwire.MilliSatoshi | |||
for _, htlc := range invoice.Htlcs { | |||
_, err := updateHtlc( | |||
timestamp, htlc, invpkg.ContractSettled, nil, | |||
if htlc.State == invpkg.HtlcStateSettled { |
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.
Sorry this was indeed wrong. Fixed.
valueLen := copy(indexKey[:], k.invoiceNum) | ||
|
||
if setID != nil { | ||
valueLen += copy(indexKey[valueLen:], setID[:]) |
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.
Let's keep it for readability.
invoices/update_invoice.go
Outdated
AcceptTime: updateTime, | ||
State: HtlcStateAccepted, | ||
CustomRecords: htlcUpdate.CustomRecords, | ||
AMP: htlcUpdate.AMP.Copy(), |
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.
Ok this is now fixed.
// InvoiceDB instance. The purpose of this test is to be able to run all tests | ||
// with a custom DB instance, so that we can test the same logic with different | ||
// DB implementations. | ||
func TestInvoiceRegistry(t *testing.T) { |
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 it's great to be able to run the quick and simple to debug unit tests on all backends. This ha been essential for me when porting invoice db to SQL, otherwise i guess iteration speed would have been much slower.
I see your concern though, but I feel like given that the InvoiceDB
interface itself is defined in the invoices
package it's probably best to keep the tests here and implementation details elsewhere. Another option (for the future) might be to move the k/v implementation under invoices/kvdb
and the sql implementation under invoices/sql
.
This commit is a small refactor to move all actual DB updates after an invoice state is update to separate methods. This is a small preliminary change before we completely decouple DB updates from in-memory invocie update.
With this refactor updateHtlc is renamed to getUpdatedHtlcState and changed such that it won't change the HTLC's state and resolve time but instead returns whether the change is needed. This change is part of a multi-commit refactor to ensure that all changes to the invoice will be tracked individually.
This commit turns updateInvoiceState "const" by moving preimage update out of the function while also removing it to getUpdatedInvoiceState.
This change moves the HTLC state change out of the cancelSingleHtlc function. This is part of the larger refactor of collecting all changes to be later applied by the invoice updater.
With this commit updateInvoiceAmpState becomes getUpdatedInvoiceAmpState which will only return the new AMP state but that needs to be applied at the call site. This is a part of a larger refactor to gather all mutations of an invoice update to be later applied by the invoice updater.
This commit introduces the InvoiceUpdater interface which is meant to abstract and assist the in-memory invoice update procedure with the accompanying database updates. These abstract updater steps will enable further refactoring later while also ensuring that a full SQL implementation of the InvoiceDB interface will be possible.
With the introducation of the `InvoiceUpdater` interface we are now able to move the non-kv parts of `UpdateInvoice` completely under the invoices package. This is a preprequisite for being able to use the same code-base for the sql InvoiceDB implementation of UpdateInvoice.
This commit extracts the InvoiceDB construction from all invoice and registry tests such that we can later on run subtests with multiple backends without needing to use tags.
e3446dc
to
bcc6a3f
Compare
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🌮 Thanks again for taking the effort!
PS, linter failed with no space left on device
, seems like the CI ran out of resource?
valueLen := copy(indexKey[:], k.invoiceNum) | ||
|
||
if setID != nil { | ||
valueLen += copy(indexKey[valueLen:], setID[:]) |
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.
by readability you mean to keep it so it's more like a pure code move than any functionality change?
@positiveblue: review reminder |
Linter looks good now after a kick! |
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 👑
Reviewed 5 of 5 files at r5, 4 of 4 files at r6, 10 of 10 files at r7, all commit messages.
Reviewable status: all files reviewed, 83 unresolved discussions (waiting on @bhandras, @positiveblue, @yyforyongyu, and @ziggie1984)
This PR is a collection of small changes refactoring the
UpdateInvoice
call and alsoInvoiceDB
andInvoiceRegistry
tests to be able to makeInvoiceDB
fully implementable both in k/v and SQL versions while testing with the same unit tests.Part of: #6288
This change is