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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

A significant Refactor of Ledger Query Service Client #756

Merged
merged 12 commits into from Feb 21, 2019

Conversation

Projects
None yet
3 participants
@LLFourn
Copy link
Contributor

LLFourn commented Feb 12, 2019

The motivation for these changes is the following:

  • The layer that creates a query should consume the results of the query (rather than passing up a query and letting the generic layer consume the results)
  • We should be able to have different types of queries for different ledgers

The code previously was overly generic and restrictive (it was not doing "Tell don't ask"). Things kept getting passed back and forth between layers. For example:

  1. The lower layer was asked for the "Query" and then the upper (generic) layer got the results
  2. With the resulting transaction the upper layer then sent it down again and asked the lower layer for various things through generic traits like ExtractSecret and FindHtlcLocation.

These traits are all gone 馃帀 and we simply have a trait HtlcEvents<L: Ledger, A: Asset> that returns a Future for each type of event.

The de-generification of this did create some duplication that could be solved but I didn't see the need to address this right now. When we start using the new query types for Ethereum that @bonomat worked on they may diverge more.

Other things:

  • Removed most "validation" code -- The LQS already guarantees things for us. We have to trust it. There's no point in duplicating checks like does the contract bytecode match the desired contract bytecode when that's exactly what we queried for! IMO We should offload more validation into the query e.g query for an amount greater than or equal to what was requested.

  • Deleted the QueryCache. After looking at it I don't think it was the best approach in the end. Now to make duplicate queries after a comit node restart we can just implement HtlcEvents for something with an inner QueryBitcoin or QueryEthereum that is idempotent because it stores the query for a particular event in a database.


Because of the merge, this PR now also:

Resolves #763.

@wafflebot wafflebot bot added the review label Feb 12, 2019

/// deduplicated results into a stream.
// Here in case we want to use it later
#[allow(dead_code)]
pub fn poll_future_into_stream<

This comment has been minimized.

@LLFourn

LLFourn Feb 12, 2019

Author Contributor

Note: this is here in case we want to be able to return query results as a stream (before implementing websockets).

This comment has been minimized.

@D4nte

D4nte Feb 17, 2019

Member

Remove until actually used.

@thomaseizinger
Copy link
Member

thomaseizinger left a comment

Here are some initial comments. Need to think about this a bit more.

I am not really happy with all the traits + generic implementation of LqsEvents going away.
The reason is that now, actually crucial logic that should be shared across multiple ledgers is duplicated. In fact, you have to do many things again in exactly the same way if you want to implement an new ledger.

Regarding the QueryCache: Did you verify that even without a restart, only 1 query is created under all circumstances? The reason it existed is not to facilitate restarts. It is to guarantee that we don't miss a transaction by accidentally creating a 2nd query in a 2nd invocation of the poll where the query is created.

mut f: F,
) -> Box<dyn Future<Item = I, Error = E> + Send + 'static> {
let ticker = Interval::new(Instant::now(), poll_interval)
.map_err(|e| unreachable!("Interval cannot error {:?}", e))

This comment has been minimized.

@thomaseizinger

thomaseizinger Feb 12, 2019

Member

It can, be if it does, something else is wrong.
Still not sure if we should actually panic at this point.
I am tending more and more to not since it can happen in production and we should shutdown the node because of that.

Should we track that with an issue?

Suggested change
.map_err(|e| unreachable!("Interval cannot error {:?}", e))
.map_err(|e| unreachable!("Interval shouldn't error {:?}", e))

This comment has been minimized.

@D4nte

D4nte Feb 17, 2019

Member

I'd add a TODO (Prod): avoid panic

Show resolved Hide resolved application/comit_node/src/swap_protocols/rfc003/bitcoin/htlc_events.rs
Show resolved Hide resolved application/comit_node/src/swap_protocols/rfc003/ethereum/htlc_events.rs Outdated
Show resolved Hide resolved application/comit_node/src/swap_protocols/rfc003/ethereum/htlc_events.rs Outdated
Show resolved Hide resolved ...tion/comit_node/src/swap_protocols/rfc003/events/ledger_event_futures.rs Outdated
Show resolved Hide resolved ...tion/comit_node/src/swap_protocols/rfc003/events/ledger_event_futures.rs Outdated
Show resolved Hide resolved application/comit_node/src/swap_protocols/rfc003/ethereum/htlc_events.rs Outdated
@D4nte

This comment has been minimized.

Copy link
Member

D4nte commented Feb 12, 2019

Here are some initial comments. Need to think about this a bit more.

I am not really happy with all the traits + generic implementation of LqsEvents going away.
The reason is that now, actually crucial logic that should be shared across multiple ledgers is duplicated. In fact, you have to do many things again in exactly the same way if you want to implement an new ledger.

Regarding the QueryCache: Did you verify that even without a restart, only 1 query is created under all circumstances? The reason it existed is not to facilitate restarts. It is to guarantee that we don't miss a transaction by accidentally creating a 2nd query in a 2nd invocation of the poll where the query is created.

@LLFourn I agree with both these comments. Not sure worth reviewing until this is settled.

@thomaseizinger

This comment has been minimized.

Copy link
Member

thomaseizinger commented Feb 12, 2019

Regarding the QueryCache: Did you verify that even without a restart, only 1 query is created under all circumstances? The reason it existed is not to facilitate restarts. It is to guarantee that we don't miss a transaction by accidentally creating a 2nd query in a 2nd invocation of the poll where the query is created.

Note that the current (my) solution is super ugly and I'd rather replace it with something much more explicit. I just didn't know how :)

@thomaseizinger

This comment has been minimized.

Copy link
Member

thomaseizinger commented Feb 12, 2019

We should offload more validation into the query e.g query for an amount greater than or equal to what was requested.

I disagree. I think LQS should return us the transaction that is very likely the attempt of the other person to do what they meant to do. The COMIT node can then verify if it actually has the correct amount.

If we offload the amount into the query, we will miss such an attempt and the state machine will never move forward but also the other party will never send a 2nd transaction.

A significant Refactor of Ledger Query Service Client
The motivations I attempted to carry out were:

- The code creating the query should consume the queries results and
  extract the information from the transaction

- We should be able to have different types of queries for different
  ledgers

Other things:

- Removed most "validation" code -- The LQS already guarantees things
  for us. We have to trust it. There's no point in duplicating checks
  like does the contract bytecode match the desired contract bytecode
  when that's exactly what we queried for! IMO We should offload more
  validation into the query e.g query for an amount greater than or
  equal to what was requested.

- Deleted the `QueryCache`. After looking at it I don't think it was
  the best approach in the end. Now to make duplicate queries after a
  comit node restart we can just implement `HtlcEvents` for something
  with an inner `QueryBitcoin` or `QueryEthereum` that is idempotent
  because it stores the query for a particular event in a database.

@LLFourn LLFourn force-pushed the 638-refactor-lqs branch from fbe0802 to e85bb79 Feb 14, 2019

@LLFourn

This comment has been minimized.

Copy link
Contributor Author

LLFourn commented Feb 14, 2019

I am not really happy with all the traits + generic implementation of LqsEvents going away.
The reason is that now, actually crucial logic that should be shared across multiple ledgers is duplicated. In fact, you have to do many things again in exactly the same way if you want to implement an new ledger.

With your feedback I've constructed it so the query making processing code no longer has to check that the HTLC was funded with the correct asset. The way I did it seems to have worked out well because now you have to return (in the future) the asset that was funded so it's very hard accidentally avoid the check (we have to fudge the ERC20 one until we have the new receipt api).

Regarding the QueryCache: Did you verify that even without a restart, only 1 query is created under all circumstances? The reason it existed is not to facilitate restarts. It is to guarantee that we don't miss a transaction by accidentally creating a 2nd query in a 2nd invocation of the poll where the query is created.

Yes only one query is created under all circumstances as the thing that is polled only asks for the future once. Hopefully we can re-architect the state machine itself to guarantee this for us.

@LLFourn LLFourn force-pushed the 638-refactor-lqs branch from e85bb79 to d2474d4 Feb 15, 2019

Validate assets generically in the state machine
It is now the state_machine's responsibility to check that the asset
that is included in the "Funding" stage of an HLTC is of equal (or
greater) value to the asset that was agreed upon.

This lifts the responsibility from the query result fetching code.

+ Address feedback in on the PR in various places

@LLFourn LLFourn force-pushed the 638-refactor-lqs branch from d2474d4 to 85d1ea4 Feb 15, 2019

@D4nte
Copy link
Member

D4nte left a comment

Minor comments. good work.

Show resolved Hide resolved application/comit_node/src/ledger_query_service/mod.rs Outdated
Show resolved Hide resolved application/comit_node/src/ledger_query_service/timer_poll_future.rs Outdated
Show resolved Hide resolved application/comit_node/src/ledger_query_service/timer_poll_future.rs Outdated
/// deduplicated results into a stream.
// Here in case we want to use it later
#[allow(dead_code)]
pub fn poll_future_into_stream<

This comment has been minimized.

@D4nte

D4nte Feb 17, 2019

Member

Remove until actually used.

mut f: F,
) -> Box<dyn Future<Item = I, Error = E> + Send + 'static> {
let ticker = Interval::new(Instant::now(), poll_interval)
.map_err(|e| unreachable!("Interval cannot error {:?}", e))

This comment has been minimized.

@D4nte

D4nte Feb 17, 2019

Member

I'd add a TODO (Prod): avoid panic

thomaseizinger added some commits Feb 20, 2019

Remove poll_future_into_stream and poll_dedup
Can be retrieved from history at a later point if we actually need it.
Streamline naming of events
The events FundedTransaction etc are all renamed to no longer have
the word "Transaction" in them. Reason is that they actual model the
_event_ that happens and the transaction is just a field that is
recorded as part of the event.

To avoid nameclashes, the corresponding Future types have been
postfixed with "Future".

Some variables names have also need adjusted.
@D4nte

D4nte approved these changes Feb 20, 2019

@thomaseizinger thomaseizinger merged commit 438f9f3 into master Feb 21, 2019

1 check passed

license/cla Contributor License Agreement is signed.
Details

@thomaseizinger thomaseizinger deleted the 638-refactor-lqs branch Feb 21, 2019

@wafflebot wafflebot bot removed the review label Feb 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.