Add graphql branch and breaking changes #2987

ecioppettini · 2021-02-02T20:09:13Z

This branches from #2981 instead of master because that contains the core logic needed for this to work, but I think the separation is helpful because:

a) This is the PR that involves the graphql breaking changes, so merging the previous one by itself is a bit safer.
b) This can be reviewed independently in general, as just looking at the schema.graphql diff gives an idea of what this does. The jormungandr/explorer/graphql/mod.rs serves the same purpose, although it's a bit more verbose as I had to move entire functions to the Branch type, and that causes a big diff even if the implementation is basically the same.

Idea

With these changes it should be possible to:

Know which block is the current tip.
Know which blocks are considered tips (which means, which blocks are the branches).
Make queries contextualized to a branch, either just whatever the current one is, or by providing a hash.

Breaking changes

allBlocks (now just blocks, probably should change the other ones too), allStakePools, allVotePlans are now in Branch instead of Query (the graphql root)
Address { transactions } is now in Branch too, under transactionsByAddress.
Same thing for Epoch { blocks }, under blocksByEpoch
Transaction { block } is now Transaction { blocks } (in plural), so it returns a list instead.
blockByChainLength is now blocksByChainLength (in plural), so it returns [Block!]! instead of Block.
status is now settings and Status is now Settings, as it was a weird type anyway. It also doesn't contain the latest block, as that can be queried through Query { mainTip { block { .. } } } now.

Non-breaking changes

mainTip and tips. These allow to retain the current behavior of most queries by using mainTip to scope the results.
branch can be used to get a branch by it's hash. This will return a not found error if the branch is not in memory anymore.
Add Address { confirmed_transactions }. Although, this is a bit extra.

Conceptual

This introduces a Branch graphql type. It's identified by the hash of a Block. The difference between the two is that the Block is the block by itself, let's say, a collection of transactions with a parent pointer. The Branch represents the multiverse state produced by the application of that block.
The distinction is mostly because the idea is to garbage collect the multiverse, but that also includes states that are in the mainchain, so you may have a Block (on disk, when we implement that) that has no Branch (in memory), which means we can't answer queries that depend on the state at that point (tx's by address, delegation, let's say). Although, it's probably possible to design a db (using chain length to timestamp/filter) schema that can account for this, I'm not sure if it is an actual use-case.

this way, it's possible to get all tips/main tip, and make a query in any of those. other changes: - Transaction { block } is now Transaction { blocks } - blockByChainLength is now blocksByChainLength - Block has a Branches field

it's not really necessary, as getting the confirmed branch and getting it from there creates less api redundancy also, update the schema

also, the usage of `first` in the allBlocks query didn't really make sense, as using `first` and `last` at the same time doesn't really make sense

mzabaluev · 2021-03-04T08:21:33Z

jormungandr/src/explorer/graphql/mod.rs

+        Block {
+            hash: block.id(),
+            contents: tokio::sync::Mutex::new(Some(block)),
+        }
    }

    async fn get_explorer_block(&self, db: &ExplorerDB) -> FieldResult<Arc<ExplorerBlock>> {


Could rename to fetch_explorer_block to reflect that it caches the retrieved block now.

It's more a lazy init than a cache, the only reason to do this is to not fetch the block unless some of the required fields need it, and it's saved so that if two fields need it then it's not fetched twice.

I agree with the name change anyway.

To be honest this doesn't really belong in this PR, especially because for block you almost always want the block anyway, except in the rare case that you ask for a paginated list of hashes but don't ask for any fields.

mzabaluev · 2021-03-04T08:28:23Z

jormungandr/src/explorer/graphql/mod.rs

    }

    async fn get_contents(&self, context: &Context) -> FieldResult<ExplorerTransaction> {
        if let Some(c) = &self.contents {
            Ok(c.clone())
        } else {
-            let block = self.get_block(context).await?;
+            //TODO: maybe store transactions outside blocks? as Arc, as doing it this way is pretty wasty
+            let block = &self.get_blocks(context).await?[0];


Can there be a get_block operation if you already know you can use a block with the specific hash?

I'm not sure if I understand. There will always be at least a block there, you mean a function that gets that one?

To be honest, this query is not designed really well, getting the block to get the transaction body is kinda bad, because it also means transactions are duplicated if they are in multiple blocks, which is kinda a waste of memory.

Fetching the entire block to get only one transaction it's probably no ideal either.

I didn't want to beat around the bush too much in this PR, so I just left what has equivalent behavior to the previous thing. (Also, extracting transaction bodies from the blocks is not that trivial anyway, because of gc and other things).

I mean it's already wasteful to retrieve several blocks if you only need one, and you can just pick the first of self.block_hashes and query just that block by its hash.

Optimizing to be able to query individual fragments straight from the store/db would be good (and might be possible with some zero-copy block storage access and iteration on raw fragments in a raw block), but this is a topic for a more substantial rework that would involve the storage layer, so not for this PR. Cc @eugene-babichenko

oh, that... Yeah, sure, makes sense.

Well, considering that the explorer doesn't even use the storage, yeah, that's a big change. I'm not sure we want to do that, though? And using raw fragments wouldn't play that nice with the way we show utxo inputs now, I guess.

mzabaluev · 2021-03-04T08:30:36Z

jormungandr/src/explorer/graphql/mod.rs

@@ -1328,6 +1520,30 @@ impl Query {
        Transaction::from_id(id, context).await
    }

+    /// get all current tips, sorted (descending) by their length
+    async fn tips(&self, context: &Context) -> Vec<Branch> {


We use tip in the node protocol for the currently selected branch.
Maybe use branch_heads or just branches, considering the return type is Vec<Branch>?

So, now that you bring that up... I guess what I'm calling Branch in the explorer/api is more like what's called Ref in the ledger.

I'm not entirely convinced that name makes sense for the public api, though.

And also I don't think that queries should be limited to heads.

But maybe it makes sense to call the thing Ref, StateRef, or something like that (not commit unfortunately).

Ref is OK in internal code, but for the public explorer API we should stick to the nomenclature used elsewhere. I think "braches" are close in meaning to the concept used in git (and git has a HEAD alias for what we mean by the "tip").

mzabaluev · 2021-03-04T08:31:15Z

jormungandr/src/explorer/graphql/mod.rs

+
+    /// get the block that the ledger currently considers as the main branch's
+    /// tip
+    async fn main_tip(&self, context: &Context) -> Branch {


So this is just tip in the node protocol, let's be consistent.

mzabaluev · 2021-03-04T08:35:38Z

testing/jormungandr-testing-utils/resources/explorer/graphql/schema.graphql

  transaction(id: String!): Transaction!
+
+  """get all current tips, sorted (descending) by their length"""
+  tips: [Branch!]!


So this should be branches to be consistent with the node protocol's terminology...

mzabaluev · 2021-03-04T08:35:48Z

testing/jormungandr-testing-utils/resources/explorer/graphql/schema.graphql

+  get the block that the ledger currently considers as the main branch's
+  tip
+  """
+  mainTip: Branch!


And this is tip.

mzabaluev · 2021-03-04T08:36:25Z

testing/jormungandr-testing-utils/resources/explorer/graphql/schema.graphql

+type VotePayloadPrivateStatus {
+  proof: String!
+  encryptedVote: String!
+}
+
 type VotePayloadPublicStatus {
  choice: Int!
 }

-union VotePayloadStatus = VotePayloadPublicStatus
+union VotePayloadStatus = VotePayloadPublicStatus | VotePayloadPrivateStatus



Good, we missed that.

jormungandr/src/explorer/graphql/mod.rs

More descriptive message for an explorer error when the block containing a fragment/transaction cannot be fetched.

ecioppettini added breaking-change A-explorer Area: Explorer API and backend labels Feb 2, 2021

ecioppettini self-assigned this Feb 2, 2021

ecioppettini force-pushed the add-graphql-branch-and-breaking-changes branch from b80b7f7 to 664a300 Compare February 8, 2021 13:59

ecioppettini marked this pull request as ready for review February 8, 2021 14:00

ecioppettini mentioned this pull request Feb 13, 2021

Migrate juniper to async graphql #3022

Merged

ecioppettini force-pushed the add-graphql-branch-and-breaking-changes branch 2 times, most recently from 21ffb77 to 4c44623 Compare February 19, 2021 18:38

ecioppettini added 7 commits March 1, 2021 12:47

add Branch graphql type and breaking changes

c385de4

this way, it's possible to get all tips/main tip, and make a query in any of those. other changes: - Transaction { block } is now Transaction { blocks } - blockByChainLength is now blocksByChainLength - Block has a Branches field

change integration tests graphql queries

7e6286b

dedup blocks by transaction

55071ac

re-add epoch_stability_depth to Settings

51fd0f4

remove graphql address { confirmed_transactions }

1d8df58

it's not really necessary, as getting the confirmed branch and getting it from there creates less api redundancy also, update the schema

fix integration tests too

faccf84

fix more test queries

456ccb3

also, the usage of `first` in the allBlocks query didn't really make sense, as using `first` and `last` at the same time doesn't really make sense

ecioppettini force-pushed the add-graphql-branch-and-breaking-changes branch from 4c44623 to 456ccb3 Compare March 1, 2021 21:58

mzabaluev reviewed Mar 4, 2021

View reviewed changes

ecioppettini added 2 commits March 4, 2021 21:06

rename main_tip to tip and tips to branches

96f7c28

fetch only one block for transaction contents

253ddb4

mzabaluev approved these changes Mar 8, 2021

View reviewed changes

jormungandr/src/explorer/graphql/mod.rs Outdated Show resolved Hide resolved

Edited an error string

b7f0da0

More descriptive message for an explorer error when the block containing a fragment/transaction cannot be fetched.

mzabaluev merged commit bfc8955 into master Mar 8, 2021

mzabaluev deleted the add-graphql-branch-and-breaking-changes branch March 8, 2021 08:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add graphql branch and breaking changes #2987

Add graphql branch and breaking changes #2987

ecioppettini commented Feb 2, 2021

mzabaluev Mar 4, 2021

ecioppettini Mar 4, 2021

mzabaluev Mar 4, 2021

ecioppettini Mar 4, 2021

mzabaluev Mar 5, 2021

ecioppettini Mar 5, 2021

mzabaluev Mar 4, 2021

ecioppettini Mar 4, 2021

mzabaluev Mar 5, 2021

mzabaluev Mar 4, 2021

mzabaluev Mar 4, 2021

mzabaluev Mar 4, 2021

mzabaluev Mar 4, 2021

Add graphql branch and breaking changes #2987

Add graphql branch and breaking changes #2987

Conversation

ecioppettini commented Feb 2, 2021

Idea

Breaking changes

Non-breaking changes

Conceptual

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment