Report system chunk transactions within a block

[m-Peter]

Closes #911

Description

The last transaction returned from flow.GetTransactionsByBlockID, is the system chunk transaction. We add it as the last transaction in the last collection, so it can be returned from the command:

flow blocks get 53665542 --network=mainnet --include transactions

---

For contributor use:

• Targeted PR against master branch
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work
• Code follows the standards mentioned here
• Updated relevant documentation
• Re-reviewed Files changed in the Github PR explorer
• Added appropriate labels

[codecov-commenter]

Codecov Report

Merging #1123 (a66cbe5) into master (daa1550) will increase coverage by 0.01%.
The diff coverage is 41.66%.

@@            Coverage Diff             @@
##           master    #1123      +/-   ##
==========================================
+ Coverage   39.28%   39.30%   +0.01%     
==========================================
  Files          37       37              
  Lines        1876     1888      +12     
==========================================
+ Hits          737      742       +5     
- Misses       1051     1056       +5     
- Partials       88       90       +2     

Flag	Coverage Δ
unittests	39.30% <41.66%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
internal/blocks/get.go	65.90% <41.66%> (-9.10%)	⬇️

[bjartek]

Is it useful to have this here, currently there is no way of getting that transaction using flow cli. Having the ability to get the system chunk transaction for a given height would be very useful though.

[sideninja]

@m-Peter should we merge this?

[m-Peter]

@sideninja I have re-opened this PR frorm my personal GitHub account. The previous PR was here: #1091. @janezpodhostnik Had a good remark #1091 (comment), so I guess we should wait to see whether he can propose an alternative.

[janezpodhostnik]

This is a tougher problem than I thought. I'll open another issue in the flow-go repo and try to get some more feedback. Sorry for the holdup.

[peterargue]

FWIW, I think this could be made more efficient by flipping the logic. Instead of requesting each collection then calling GetTransactionsByBlockID, just call GetTransactionsByBlockID. You could then group the transactions by collection using the IDs within the block's collection guarantee. Any tx that aren't in the guarantee (there should only be 1) are in the system collection

[m-Peter]

@peterargue I had already tried this, but the models involved here are the ones coming from flow-go-sdk, e.g:

• flowsdk.Transaction
• flowsdk.TransactionResult
• flowsdk.Collection
• flowsdk.CollectionGuarantee

The flowsdk.Transaction & flowsdk.TransactionResult contain no information about collection ID, and flowsdk.CollectionGuarantee contains only the CollectionID. No information about TransactionIDs, so I'm not sure how these can be grouped by.

Let me know if you had something else in mind 🙏

[peterargue]

@peterargue I had already tried this, but the models involved here are the ones coming from flow-go-sdk, e.g:

• flowsdk.Transaction
• flowsdk.TransactionResult
• flowsdk.Collection
• flowsdk.CollectionGuarantee

The flowsdk.Transaction & flowsdk.TransactionResult contain no information about collection ID, and flowsdk.CollectionGuarantee contains only the CollectionID. No information about TransactionIDs, so I'm not sure how these can be grouped by.

Let me know if you had something else in mind 🙏

Ah, you're right. Looks like the needed change was never backported. We just merged PR onflow/flow-go-sdk#436 which adds CollectionID to the TransactionResult responses.

[m-Peter]

That's great 🙌 I will update to your suggestion, once [email protected] is in this repo 👍

[sideninja]

@m-Peter this is blocked by the Go SDK update? cc @chasefleming

[chasefleming]

I think we're waiting on v0.43.0 for the go sdk, is that correct still @m-Peter ?

[m-Peter]

I tried bumping onflow/flow-go-sdk to v0.43.0, but I am getting some build errors:

../../go/pkg/mod/github.com/onflow/[email protected]/model/bootstrap/node_info.go:85:37: cannot use info.SigningAlgorithm (variable of type "github.com/onflow/crypto".SigningAlgorithm) as "github.com/onflow/flow-go/crypto".SigningAlgorithm value in argument to crypto.DecodePrivateKey
# github.com/onflow/flow-go/utils/grpcutils
../../go/pkg/mod/github.com/onflow/[email protected]/utils/grpcutils/grpc_client.go:25:43: cannot use publicFlowNetworkingKey (variable of type "github.com/onflow/crypto".PublicKey) as "github.com/onflow/flow-go/crypto".PublicKey value in argument to DefaultClientTLSConfig: "github.com/onflow/crypto".PublicKey does not implement "github.com/onflow/flow-go/crypto".PublicKey (wrong type for method Algorithm)
		have Algorithm() "github.com/onflow/crypto".SigningAlgorithm
		want Algorithm() "github.com/onflow/flow-go/crypto".SigningAlgorithm

[m-Peter]

I think flow-go should bump the flow-go-sdk to @v0.43.0 first.

[bjartek]

FWIW, I think this could be made more efficient by flipping the logic. Instead of requesting each collection then calling GetTransactionsByBlockID, just call GetTransactionsByBlockID. You could then group the transactions by collection using the IDs within the block's collection guarantee. Any tx that aren't in the guarantee (there should only be 1) are in the system collection

But it is always the latest one that is the system chunk transaction is it not?

[bjartek]

It would also be very nice to have a way to get system transaction events from a block height. Without getting all the other data for that block.

[peterargue]

But it is always the latest one that is the system chunk transaction is it not?

yes

It would also be very nice to have a way to get system transaction events from a block height. Without getting all the other data for that block.

There's a community team working on onflow/flow-go#4584 now. The end result will be 2 new API endpoints to get the service transaction and result

[sideninja]

@m-Peter should we get this merged?

[m-Peter]

@sideninja I still have to address this suggestion (#1123 (comment)) from Peter. I need to check if that addition is included in the current flow-go-sdk version.

[peterargue]

another update for the flow-go side. onflow/flow-go#4584 was merged adding 2 new methods: GetSystemTransaction and GetSystemTransactionResult. It most likely will be deployed sometime mid/late Jan

[chasefleming]

@m-Peter what's the status on this? Can this be merged yet?