Skip to content
This repository has been archived by the owner on Dec 2, 2024. It is now read-only.

PLT-359 ADR regarding problem with PAB's integration with indexing solutions. #550

Merged

Conversation

koslambrou
Copy link
Contributor

@koslambrou koslambrou commented Jun 28, 2022

ADR regarding problem with PAB's integration with indexing solutions.

This was triggered by the issue #473. A temporary fix was submitted by #544, but the ideal solution should be the one presented in this ADR.

The ADR is found here: https://plutus-apps--550.org.readthedocs.build/en/550/adr/0004-pab-indexing-solution-integration.html

Pre-submit checklist:

  • Branch
    • Tests are provided (if possible)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
    • Formatting, PNG optimization, etc. are updated
  • PR
    • Self-reviewed the diff
    • Useful pull request description
    • Reference the ADR in the PR and reference the PR in the ADR (if revelant)
    • Reviewer requested

@koslambrou koslambrou marked this pull request as ready for review June 28, 2022 14:39
@koslambrou
Copy link
Contributor Author

Thanks @joseph-fajen :)

@koslambrou koslambrou force-pushed the PLT-359-write-adr-for-pab-indexing-solution-integration branch 2 times, most recently from 160f239 to b19f403 Compare June 28, 2022 18:17
Copy link
Contributor

@joseph-fajen joseph-fajen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had just a few minor comments. Looks good!

Copy link
Contributor

@raduom raduom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

doc/adr/0002-pab-indexing-solution-integration.rst Outdated Show resolved Hide resolved
doc/adr/0002-pab-indexing-solution-integration.rst Outdated Show resolved Hide resolved

- Hard to implement
- As fast as the slowest indexing solution
- Can not be feasible with external indexing solutions for which we can’t control the indexing speed (e.g. Blockfrost_ or Scrolls_)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While true, I don't think that anyone would expect that we would synchronise with external indexing solutions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but I've added it in case we ever get a requirement which says that the PAB should be able to integrate with external indexing solutions.

Query functions should interact with a single source of truth
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

In this scenario, we make the design decision that the Contract API should only interact with a single indexing solution. Thus, any blockchain information currently stored in the PAB should be moved to the indexing solution. Also, combining indexing solutions would need to be integrated in the single indexing solution that’s connected to the PAB.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes the most sense.

doc/adr/0002-pab-indexing-solution-integration.rst Outdated Show resolved Hide resolved
doc/adr/0002-pab-indexing-solution-integration.rst Outdated Show resolved Hide resolved
@koslambrou koslambrou force-pushed the PLT-359-write-adr-for-pab-indexing-solution-integration branch from b19f403 to c2c590b Compare June 30, 2022 17:46
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overall impression is that we have a choice between two approaches:

  1. To hide the synchronization details (slot information of different components) in the indexing part or behind the methods with smart waiting.
  2. To provide the details and let the user decide how to deal with the synchronization of states.

I suppose the best way to have both of them, as it depends on the use cases. At the beginning, while learning or for simple use cases, it might be enough the standard solution, but later more flexibility might be needed (but which will be too complex at the beginning).

Comment on lines 125 to 139
Query functions should interact with a single source of truth
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

In this scenario, we make the design decision that the Contract API should only interact with a single indexing solution.
Thus, any blockchain information currently stored in the PAB should be moved to the indexing solution.
Also, combining indexing solutions would need to be integrated in the single indexing solution that’s connected to the PAB.

Pros:

- Simplest in design to implement (other than manual work to move code)
- No modification to the Contract API

Cons:

- PAB won't be able to integrate with external indexing solutions (e.g. Blockfrost_ or Scrolls_), but these external solutions would need to be integrated into the single source of truth indexing solution. Therefore, this solution reduces the PAB's flexibility.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I think I prefer this option (should we show our preference here in comments?). It separates the semantics of the components. Indexing happens in one place, running contracts in another one.

Therefore, this solution reduces the PAB's flexibility.

I don't think so. It just moves all indexing to one place and it's up to the user to use the indexing solution he prefers. It just means that we should make sure that 3rd-party indexing solution are supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[...] (should we show our preference here in comments?)

Good points about preference. I think it's okay to leave them here as comments. I'll add the PR in the "Notes" section of the ADR.

It separates the semantics of the components. Indexing happens in one place, running contracts in another one.

Does this contradict what you said in the previous comment? You say here that you prefer the "single source of truth" approach, but in your previous comment you say that we should allow (eventually) users to deal with state synchronization of components. But if we have a single source of truth, then we don't need to deal with state synchronization of components.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, at first I wrote the comment in this thread, choosing between the proposed solutions.

And later I tried to reflect about some general solution, what approach is better for us and how it fits the PAB's vision I suppose.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO whathever indexing solution PAB uses should be hidden from the user. So unspentTxOutFromRef (~ a PAB function) should be in charge of making sure plutus-chain-index has caught up with the rest of PAB.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andreabedini I'm interpreting this as solution 1 of this ADR (Make sure all components are in sync), but it might be different. You're saying that all calls to PAB functions in the Contract Monad should wait for all other components (like chain-index) to be synced and, similarly, calls to chain-index funcions should wait for all other components to be synced.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@koslambrou isn't it "Query functions should interact with a single source of truth" ? maybe I am missing some details.

You're saying that all calls to PAB functions in the Contract Monad should wait for all other components (like chain-index) to be synced and, similarly, calls to chain-index funcions should wait for all other components to be synced.

Yes, this is what I am thinking.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's what you're thinking, then you're talking about the category "Make sure all components are in sync". With the "Query functions should interact with a single source of truth" you don't need to worry about functions not being in sync, because there is only ONE component which indexes information (all the indexing information from the PAB would be moved to that single source of truth indexing component).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@koslambrou The way I see it is: from the POV of the contract monad, there is only one source of truth, the interpreter which is run by PAB. The interpreter giving inconsistent results is, IMHO, 100% an interpreter bug.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, I agree with the decisions in this ADR so perhaps we are just using different words to express the same concept.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@koslambrou The way I see it is: from the POV of the contract monad, there is only one source of truth, the interpreter which is run by PAB. The interpreter giving inconsistent results is, IMHO, 100% an interpreter bug.

Good point! I understand what you meant now.

@koslambrou koslambrou mentioned this pull request Jul 13, 2022
8 tasks
@koslambrou koslambrou force-pushed the PLT-359-write-adr-for-pab-indexing-solution-integration branch 2 times, most recently from 47b62ca to 7e005f0 Compare July 14, 2022 13:39
@koslambrou koslambrou requested review from a user and raduom July 14, 2022 17:25
@koslambrou koslambrou force-pushed the PLT-359-write-adr-for-pab-indexing-solution-integration branch from 05bc603 to 605286b Compare July 14, 2022 18:36

* We will add new indexers in Marconi in order to replicate the information indexed by `plutus-chain-index`

* We will adapt the architecture of Marconi (which will become our new indexing component) to support waiting queries
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this mean, exactly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, we have only implemented indexers which stores information in the database. But the indexer also have a notification field, right? Then this notification field allows an indexer to support waiting queries (like awaitTxConfirmed).

I used the word adapt because we haven't officially made a decision on how the notification system will work. How would you rephrase it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The phrasing is fine, I was just curious what you meant. You are making it sound a bit like a teaser of things to come in the next episode. Which is fine by me. :)

doc/adr/0004-pab-indexing-solution-integration.rst Outdated Show resolved Hide resolved

Cons:

- PAB won't be able to integrate with external indexing components (e.g. Blockfrost_ or Scrolls_), but these external solutions would need to be integrated into the single source of truth indexing solution. Therefore, this solution reduces the PAB's flexibility.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure this is a con. Reducing a component's responsibilities seems like a pro to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PAB won't be able to integrate with external indexing components
Was this ever in our roadmap? If not, it should not be listed either.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not in the official roadmap yet, but there is demand for using other indexing components with the PAB (Blockfrost is one of them).

Copy link
Contributor Author

@koslambrou koslambrou Jul 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure this is a con. Reducing a component's responsibilities seems like a pro to me.

Makes sense. High cohesion is always good :D

I guess more than a con, the fact the external indexing components need to integrated into the single source of truth indexing solution of more of a consequence (nor good nor bad)

Cons:

- PAB won't be able to integrate with external indexing components (e.g. Blockfrost_ or Scrolls_), but these external solutions would need to be integrated into the single source of truth indexing solution. Therefore, this solution reduces the PAB's flexibility.
- The design of the indexing component will need to be changed to support waiting queries (like the `awaitTxConfirmed` from PAB)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked what change would you envision before, because this seems to be precisely the notification system I was considering for indexers, in which case it's not a change, it's a feature that we planned.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, I'm talking about "indexing component" in general, not specifically Marconi. I'm basically saying that the indexing components needs to support waiting queries (which Marconi will). But if someone wants to replace PAB's main indexing component with another one, it needs to support waiting queries.


Cons:

- Makes the query APIs more complicated, which ultimately result in a more complex contract
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure this is the case. The contract API consists of a EDSL (effect system based), so we could implement synchronisation as a cross-cutting/semantic/interpreter concern, by specifying a 'context configuration' where the user would decide what is acceptable for their application in general.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True! But the downside of this is that is doesn't actually solve the problem in the context right? The contract would throw a runtime error if the chain-index is not fully synced. Compare that to the "single source of truth" solution were you would never get a synchronization error.

The upside is that the error would be very clear and explicit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @andreabedini noticed before, it's all about the implementation of the interpreter. You can have an interpreter that waits until all indexers have caught-up with the chain in which case you would never get a synchronisation error either.

Cons:

- Makes the query APIs more complicated, which ultimately result in a more complex contract
- Makes the contract slower if it has to wait for the indexing component to be synced to the desired `SlotRange`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is only true if we decide not to expose slots in the contract api (and have them as some sort of interpreter feature).

If we expose slot ranges to the contract api, then the contract writer will have full control over synchronisation and be able to decide on using a slot range of +/- infinity. In this case, the solution becomes a definitive pro in my book.


* We will adapt the architecture of Marconi (which will become our new indexing component) to support waiting queries

* Since HTTP APIs are not well suited to waiting queries, we will use RPC via UNIX Socket to communicate information between PAB and Marconi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not using Marconi has a library?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmm... very good point. I guess if we're using RPC via UNIX Socket, we suppose the Marconi executable is on the same machine as the PAB, therefore it makes totally to use it as a library.

Cons:

- As fast as the slowest indexing component
- Tight coupling between the indexing components meaning that if the Contract only uses chain-index requests without using requests from other indexing components, the chain-index will still have to wait for all other components to be in sync with each other
Copy link
Contributor

@andreabedini andreabedini Jul 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe the interpreter can be given the list of relevant indexers? (and only run those)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem with that is that the PAB doesn't know what the "relevant" indexers are (unless they are specified by the user when starting the PAB). However, doing it that way will make the PAB highly coupled with the builtin contracts. Might not be such a problem right now, but imagine we extend the PAB to be even more generalized, where the contracts are not builtin, but are actually separate executable files which can be injected in PAB.

Therefore, I believe PAB should not care about what requests the contract is composed of.

@koslambrou koslambrou force-pushed the PLT-359-write-adr-for-pab-indexing-solution-integration branch from 605286b to 7a6799f Compare July 27, 2022 13:24
@koslambrou koslambrou force-pushed the PLT-359-write-adr-for-pab-indexing-solution-integration branch from 7a6799f to 620fc0f Compare July 27, 2022 18:24
@koslambrou koslambrou merged commit a712802 into main Jul 28, 2022
@koslambrou koslambrou deleted the PLT-359-write-adr-for-pab-indexing-solution-integration branch July 28, 2022 02:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants