Jump to conversation

Unresolved conversations (15)

joostjager

Jul 6, 2020

Unclaimed?

Outdated

contractcourt/anchor_resolver.go

joostjager

Jul 6, 2020

Rename to `CLAIMED_FIRST_STAGE` to make it clear that this is a claim result?

lnrpc/rpc.proto

joostjager

Jul 6, 2020

I still don't fully understand why `PutResolverReport` needs to be called directly. I'd think that always calling `CheckPoint` in resolvers makes it clearer what is going on. It checkpoints the full state (internal + reports) of a resolver and that is the only function that is used. I know there is access via the arb config to other functions, but maybe that isn't a good thing anyway.

contractcourt/anchor_resolver.go

joostjager

Jul 6, 2020

Still struggling with the definition of those outcomes. Inside the context of an outgoing htlc, we can only get it or not get it.

contractcourt/htlc_timeout_resolver.go

joostjager

Jul 6, 2020

Confusing to call the timeout tx `successTx` ?

Outdated

contractcourt/htlc_timeout_resolver.go

joostjager

Jul 6, 2020

When exactly is the outcome of the first stage written to the bucket? I would expect that after the presigned tx confirms

contractcourt/htlc_success_resolver.go

joostjager

Jul 6, 2020

I miss writing of the success resolution outcome in this resolver. Shouldn't the happy path also be concluded with a write to the new bucket?

...ctcourt/htlc_incoming_contest_resolver.go

joostjager

Jul 6, 2020

So there will be two `ResolverTypeIncomingHtlc` entries in the `resolversBucket`. One with outcome `ResolverOutcomeFirstStage` and another one with `ResolverOutcomeClaimed`?

Outdated

channeldb/reports.go

joostjager

Jul 6, 2020

That it is timeout, is already encoded in the resolver type. Isn't this lost?

Outdated

channeldb/reports.go

joostjager

Jun 29, 2020

Is it possible with this structure to link the stage 1 and stage 2 entry together?

channeldb/reports.go

joostjager

Jun 29, 2020

`ResolverOutcomeLost`?

Outdated

channeldb/reports.go

joostjager

Jun 29, 2020

This is just `ResolverOutcomeClaim`?

Outdated

channeldb/reports.go

joostjager

Jun 29, 2020

If you have this function, why do you still need `PutResolverReport`? Maybe the first parameter can just be the unchanged resolver state, or nil?

Outdated

contractcourt/contract_resolvers.go

cfromknecht

Jun 17, 2020

use constant for persisted enum?

Outdated

channeldb/reports.go

joostjager

Jun 2, 2020

All of the resolvers now end with this `PutResolverReport` call. I'd try to move that to the caller of `Resolve` so that it isn't duplicated and always guaranteed to be executed. If you unify resolver report and contract report, the caller can request the report through the existing `report()` method.

Outdated

contractcourt/commit_sweep_resolver.go

Resolved conversations (40)

cfromknecht

Jul 7, 2020

```suggestion // The outcome of our on chain action that resolved the outpoint. ```

Outdated

lnrpc/rpc.proto

cfromknecht

Jul 7, 2020

👍 i do prefer these split out rather than being nested

lnrpc/rpc.proto

cfromknecht

Jul 7, 2020

```suggestion // We resolved an anchor output. ```

Outdated

lnrpc/rpc.proto

joostjager

Jul 6, 2020

~In the context of anchors, I'd call this `lost`?~ I see there is no more `Lost`

Outdated

channeldb/reports.go

joostjager

Jul 6, 2020

I noticed it isn't actually used anywhere, also not in the full diff. Is this to prevent giving meaning to the zero value?

Outdated

channeldb/reports.go

joostjager

Jun 29, 2020

Just the action of broadcasting, is that an outcome?

Outdated

channeldb/reports.go

joostjager

Jun 29, 2020

Also just `Lost`? I don't know if it is important to say why we didn't claim. There are also other reasons like cltv delta too small for which we also don't record the specific reason.

Outdated

...ctcourt/htlc_incoming_contest_resolver.go

joostjager

Jun 29, 2020

Doesn't this create the same inflexible structure that prevented adding this new data to the existing force close summary bucket in the first place? Maybe it is safer to add another nested bucket so new fields can be added later without a migration or EOF checking? Or use tlv there. Or use a separate bucket key for every field.

Outdated

channeldb/reports.go

joostjager

Jun 29, 2020

first stage `OutPoint`? to keep the reference to the field

Outdated

channeldb/reports.go

cfromknecht

Jun 26, 2020

nit: a little strange that the type name is `ResolutionResolutionType`, we could make this a top-level enum but I do like the names of the values themselves, e.g. `Resolution_ANCHOR`. not sure which is the lesser of two evils

Outdated

lnrpc/rpc.proto

cfromknecht

Jun 26, 2020

but we did not.. sweep?

Outdated

lnrpc/rpc.proto

cfromknecht

Jun 26, 2020

isn't it impossible to hit this error due to `CreateTLB` migration? couple cases where we verify the bucket's existence that we can avoid as well

Outdated

channeldb/reports.go

Roasbeef

Jun 18, 2020

Ah ok, i see now that this is the reason why.

Outdated

contractcourt/channel_arbitrator.go

Roasbeef

Jun 18, 2020

👍

lntest/itest/lnd_test.go

Roasbeef

Jun 18, 2020

I think this placement makes more sense than in `InsertUnresolvedContracts`.

contractcourt/briefcase.go

Roasbeef

Jun 18, 2020

This seems mismatched: we're inserting a _series_ of contracts here (which will each eventually generate their _own_ report), but then accept a _single_ resolver report?

Outdated

contractcourt/briefcase.go

Roasbeef

Jun 18, 2020

Can be simplified to just: `return resolvers.ForEach`.

Outdated

channeldb/reports.go

Roasbeef

Jun 18, 2020

No longer 15, needs a rebase.

Outdated

channeldb/db.go

joostjager

Jun 2, 2020

Is there anything currently being reported by the `PendingChannels` call that would also make sense to report after the channel is fully resolved, but which isn't in this struct atm?

channeldb/reports.go

joostjager

Jun 2, 2020

If the closure always persists the resolver report, can't the report just be passed in here? To make it clearer what is happening?

Outdated

contractcourt/briefcase.go

joostjager

Jun 2, 2020

Can't you add `ResolverOutcome` and `SpendTxID` to `ContractReport`? Conceptually it makes sense to me to let every resolver only have one report structure for its current state.

contractcourt/channel_arbitrator.go

joostjager

Jun 2, 2020

Clarify which outpoint this is exactly in case of two stage resolution? Same for amount.

Outdated

channeldb/reports.go

Roasbeef

May 30, 2020

Similar comment here re obtaining the value from the struct as is. Also applies to all other instances of this in the diff.

Outdated

...ctcourt/htlc_outgoing_contest_resolver.go

Roasbeef

May 30, 2020

Nice work here with the increase test coverage in this area 👍

contractcourt/htlc_success_resolver_test.go

Roasbeef

May 30, 2020

If this is just to get the amount for the HTLC itself, then this is already available in the internal `htlc` field which contains the full amount of the HTLC. The other way this information is available is from the internal `htlcResolution` struct. The sign desc contains the input value since we need that to generate a valid signature. So the path here is `h.htlcResolution.SweepSignDesc.Output.Value`.

Outdated

...ctcourt/htlc_incoming_contest_resolver.go

Roasbeef

May 30, 2020

Why not just query for the transaction directly from the `wallet`? This may require us to surface a new `wtxmgr` method.

Outdated

server.go

Roasbeef

May 30, 2020

Ah ok, the type is essentially encoded in the outcome.

Outdated

channeldb/reports.go

Roasbeef

May 30, 2020

What about an enum for the resolver _type_?

channeldb/reports.go

Roasbeef

Apr 20, 2020

Just the HTLC amount? So the value of the input, but not the second level output created.

Outdated

...ctcourt/htlc_incoming_contest_resolver.go

Roasbeef

Apr 20, 2020

Perhaps this only needs to be a part of `contractResolverKit`?

Outdated

contractcourt/channel_arbitrator.go

Roasbeef

Apr 20, 2020

To cut down on the redundant amount of methods a bit, one pattern we follow elsewhere is to make a new transaction if the passed transaction is `nil`.

Outdated

channeldb/reports.go

Roasbeef

Apr 20, 2020

The transaction should always be rolled back, we can do so easily by putting the rollback in a defer statement.

Outdated

channeldb/reports.go

Roasbeef

Apr 20, 2020

Perhaps we should also include chain fee or fee rate information?

channeldb/reports.go

Roasbeef

Apr 20, 2020

Should this also be publicly exported?

Outdated

channeldb/reports.go

joostjager

Apr 7, 2020

Will the user be able to backtrace - in case of htlcs - which htlc this was exactly? Wondering what the user flow looks like. A payment hash and/or htlc index could be useful.

channeldb/reports.go

joostjager

Apr 7, 2020

As this function doesn't know what the closures are anyway, it could maybe be just a single function pointer. Do the loop inside that function

Outdated

contractcourt/briefcase.go

joostjager

Apr 7, 2020

I think it can be worth looking into the potentially unification of this report and `c.currentReport`. Both more or less serve the same purpose, but just in different phases.

Outdated

contractcourt/anchor_resolver.go

joostjager

Apr 7, 2020

In addition to claimed, we could consider logging how much we lost too

channeldb/reports.go

joostjager

Apr 7, 2020

This isn't a final outcome. Will that ever be stored?

Outdated

channeldb/reports.go

joostjager

Apr 7, 2020

How does this structure compare to grouping the reports as a sub-bucket somewhere closer to the other channel data?

channeldb/reports.go