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

❌ Patch coverage is 50.00000% with 6 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/blocks/get.go	50.00%	4 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

[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.

[devbugging]

@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 flow-go-sdk@v0.43.0 is in this repo 👍

[devbugging]

@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/flow-go@v0.31.1-0.20230720190804-2782d45706ac/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/flow-go@v0.31.1-0.20230720190804-2782d45706ac/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

[devbugging]

@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?

[chasefleming]

@peterargue @m-Peter Any updates on this?

[chasefleming]

Closing this as I believe this was solved in a separate PR now: https://github.com/onflow/flow-cli/blob/master/internal/transactions/get-system.go

[jribbink]

We should re-prioritize considering the recent need created by scheduled callbacks (although this PR is probably stale now).

Ideally:

--include=transactions => all user + system transactions for a block. This is separate from the work already done to query individual system transactions.