centrifuge / centrifuge-chain

Centrifuge Chain: the layer-1 blockchain for real-world assets, built on Substrate.
https://centrifuge.io
GNU Lesser General Public License v3.0
182 stars 81 forks source link

lp-gateway: Drop session ID invalidation #1965

Closed cdamian closed 2 months ago

cdamian commented 2 months ago

Addressing some comments from - https://github.com/centrifuge/centrifuge-chain/pull/1961.

cdamian commented 2 months ago

Question 1. Can you elaborate on why we need the InboundEntry as an enum? Could we not have something like (Option, SessionId, DomainAddress, count), avoiding to deal with the enum variants?

The enum made sense to me since it's a lot easier to deal with a struct-like entity like this instead of a tuple. Furthermore, it creates a clear distinction between message and message proof entries. Given that we have certain rules in place for defining which router can send what, such as these, this distinction allows us to perform validation such as this.

cdamian commented 2 months ago

Question 2. How are we ensuring two proofs are not comming from the same router? Should not that information be stored somehow in the InboundEntry? (maybe a list of RouterId of the read proofs or similar)

We don't need to ensure this, if we would, we wouldn't be able to support duplicate messages.

lemunozm commented 2 months ago

struct-like entity like this instead of a tuple

Yeah, when I wrote a tuple I wanted to write mostly a struct.

We don't need to ensure this, if we would, we wouldn't be able to support duplicate messages.

I'm pretty sure we need it... This is where the "vote" concept comes into play. Suppose you have 2 duplicated messages. Then you dispatch the first one to be processed once you have at least one vote from all associated routers. At least this is how it works in Solidity.

cdamian commented 2 months ago

struct-like entity like this instead of a tuple

Yeah, when I wrote a tuple I wanted to write mostly a struct.

We don't need to ensure this, if we would, we wouldn't be able to support duplicate messages.

I'm pretty sure we need it... This is where the "vote" concept comes into play. Suppose you have 2 duplicated messages. Then you dispatch the first one to be processed once you have at least one vote from all associated routers. At least this is how it works in Solidity.

We should expect multiple proofs from the same router. Given a setup with 3 routers x, y, z, where x is supposed to send the message, y and z are supposed to send the proofs, we would expect 1 message + 2 proofs in order to process 1 message. Given this, we can have a scenario where we receive 4 proofs from y and z, 2 each, and then 2 messages from x.

Cases similar to what I described above are covered in the unit tests - here.

lemunozm commented 2 months ago

We should expect multiple proofs from the same router.

Yes, and that is where the vote for that pair (message_hash, router) increments to 2.

In the current implementation, we can receive a message from x, two proofs from y, and the message will be processed without waiting for z, or am I missing something?

cdamian commented 2 months ago

In the current implementation, we can receive a message from x, two proofs from y, and the message will be processed without waiting for z, or am I missing something?

No, we also have a test that covers the case that you mentioned here.

The logic that counts votes can be found here.

lemunozm commented 2 months ago

Ah ok, you're right! I didn't see well that part 👍🏻

So, to double-check. The current implementation can:

cdamian commented 2 months ago

Ah ok, you're right! I didn't see well that part 👍🏻

So, to double-check. The current implementation can:

  • Receive multiple same messages.
  • Receive multiple proofs for the same messages.
  • Will only process a message when at least one proof for each router is received.

Correct!

cdamian commented 2 months ago

Anyway @lemunozm, this PR is dealing with some other changes not necessarily related to votes etc. Do you mind if I merge this in the main branch and continue discussions there?

lemunozm commented 2 months ago

Yeah, sure! Having understood that, I think everything now is working as it should 👍🏻 Let's merge it

lemunozm commented 2 months ago

Also, I think now InboundEntry makes more sense to be an enum 👍🏻