Support migration of the FireFly contract from one location to another #820

awrichar · 2022-05-17T17:39:12Z

~~In a chain with #821.~~
~~Depends on hyperledger/firefly-common#8 being merged and integrated here.~~

Summary of changes:

Support a list of contract locations in the blockchain config under a new fireflyContract key.
New config example (for Ethereum):

blockchain:
- name: blockchain0
  type: ethereum
  ethereum:
    ethconnect:
      url: http://ethconnect_0:8080
      topic: "0"
    fireflyContract:
    - address: 0x057b6ae997a77726a2cfed8cbef7fd94ce42b15a
      fromBlock: 0
    - address: 0xb347e9de7d9f312c24c375fff7632e189bf1dd43

This means deprecation of the previous keys ethconnect.instance and fabconnect.chaincode.

New fireflyContract field added to namespaces.
Records the config index and address of the active contract, as well as the final event for all previously-terminated contracts. New namespace example:

{
  "id": "3ee769e3-92c5-4695-acd6-75c6e362082c",
  "name": "ff_system",
  "description": "FireFly system namespace",
  "type": "system",
  "created": "2022-05-24T19:18:40.07772Z",
  "fireflyContract": {
    "active": {
      "index": 1,
      "info": {
        "address": "0xb347e9de7d9f312c24c375fff7632e189bf1dd43",
        "fromBlock": "oldest",
        "subscription": "sb-2f4dc8bb-b577-4b9b-582b-9b5bbd347706"
      }
    },
    "terminated": [
      {
        "index": 0,
        "finalEvent": "000000000007/000000/000000",
        "info": {
          "address": "0x057b6ae997a77726a2cfed8cbef7fd94ce42b15a",
          "fromBlock": "oldest",
          "subscription": "sb-b43ec7ed-b158-48dc-473a-05bda8e0fc28"
        }
      }
    ]
  }
}

Currently there is only one globally active FireFly contract, and its info is recorded against the "ff_system" namespace. In a future PR as part of hyperledger/firefly-fir#12, each namespace will have its own blockchain plugin and therefore its own contract.

New /network/operatoraction API, with a single supported action type of terminate.
Sends a specially formatted "BatchPin" transaction to signal that all members should terminate listening to the current FireFly contract and migrate to the next one configured.

Note that this is all separate from migrating the "network rules" of a namespace (which is also in scope for FIR-12). This first step just allows migrating from one contract to another. In a future PR we can introduce the capability for a new contract to expose a "network version", so that a contract migration may also cause a rules migration (such as changing the signature of BatchPin or the rules for resolving identities).

codecov-commenter · 2022-05-17T17:51:13Z

Codecov Report

Merging #820 (620881c) into main (a425fde) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##             main      #820      +/-   ##
===========================================
+ Coverage   99.98%   100.00%   +0.01%     
===========================================
  Files         299       301       +2     
  Lines       19457     19612     +155     
===========================================
+ Hits        19455     19612     +157     
+ Misses          1         0       -1     
+ Partials        1         0       -1

Impacted Files	Coverage Δ
internal/apiserver/routes.go	`100.00% <ø> (ø)`
internal/events/event_manager.go	`100.00% <ø> (ø)`
internal/apiserver/route_post_network_action.go	`100.00% <100.00%> (ø)`
internal/blockchain/ethereum/config.go	`100.00% <100.00%> (ø)`
internal/blockchain/ethereum/ethereum.go	`100.00% <100.00%> (ø)`
internal/blockchain/ethereum/eventstream.go	`100.00% <100.00%> (ø)`
internal/blockchain/fabric/config.go	`100.00% <100.00%> (ø)`
internal/blockchain/fabric/eventstream.go	`100.00% <100.00%> (ø)`
internal/blockchain/fabric/fabric.go	`100.00% <100.00%> (ø)`
internal/database/sqlcommon/namespace_sql.go	`100.00% <100.00%> (ø)`
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a425fde...620881c. Read the comment docs.

awrichar · 2022-05-20T16:16:59Z

I think this will now need to slot in after #826

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

awrichar · 2022-05-20T20:59:16Z

Now rebased on mainline after the merge of #826

Send a specially formatted "BatchPin" transaction to signal that all members should migrate to a specific version of the contract from their config. Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

peterbroadhurst · 2022-05-20T21:13:38Z

each namespace will have its own blockchain plugin and therefore its own contract index.

I think I'm right interpreting this as:

Each unique remote namespace will have it's own blockchain plugin. Noting that there might be multiple local namespaces on the node that have separate root identities registered in that business network, and they can choose to share a single blockchain plugin, or each have a separate blockchain plugin.

awrichar · 2022-05-20T21:27:57Z

@peterbroadhurst In the situation that multiple local namespaces are actually pointed at the same remote namespace, I think they would still get multiple instances of the blockchain plugin (because each instance would map incoming events to its own local namespace). So they might "share a single blockchain plugin" in the sense that they'd each have an instance of the same plugin, but they wouldn't share a single instance of the plugin.

peterbroadhurst · 2022-05-20T21:30:03Z

k - feel like we need to net that out in FIR-12. Some of it might be terminology, but if you really have to define an individual plugin for each namespace and those configurations can't be re-used... it feels odd that you don't just define them directly in the namespace.

internal/apiserver/route_post_network_migrate.go

internal/database/sqlcommon/namespace_sql.go

internal/blockchain/ethereum/ethereum.go

awrichar · 2022-05-20T21:56:20Z

if you really have to define an individual plugin for each namespace

@peterbroadhurst yea I think it's mostly terminology here. From a configuration perspective, you can reuse a plugin configuration in many namespaces. From an internal instantiation perspective, there may actually be a separate plugin instance+event loop created for every one of those namespaces.

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

This is already covered by tracking of the current subscription ID, which will filter out events from unrecognized/old subscriptions. Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

internal/blockchain/ethereum/ethereum.go

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

internal/blockchain/ethereum/ethereum.go

peterbroadhurst · 2022-05-26T16:44:01Z

internal/blockchain/fabric/fabric.go

+
+	chaincode := termination.Info.GetString("chaincodeId")
+	f.fireflyMux.Lock()
+	if chaincode != f.fireflyChaincode {


Just an observation about us having added quite a bit of extra code duplication of logic here. At some point it feels like we need a base plugin/framework for the common code between ethereum and fabric. However, I have no objection to this going in as is - as I'm expecting you were thinking about this duplication as you went through, and how interleaved the logic is.

Yea, my gut said it wouldn't improve overall readability to factor out utilities, because so many little details are just slightly different ("address" vs "chaincodeId", for instance). It's probably an exercise worth doing at some point just to see what's possible, but I've punted (again) for now.

internal/blockchain/fabric/fabric.go

peterbroadhurst · 2022-05-26T16:47:00Z

internal/blockchain/fabric/fabric.go

+	if f.contractConfSize > 0 || contractIndex > 0 {
+		// New config (array of objects under "fireflyContract")
+		if contractIndex >= f.contractConfSize {
+			return "", "", i18n.NewError(ctx, coremsgs.MsgInvalidFireFlyContractIndex, fmt.Sprintf("blockchain.fabric.fireflyContract[%d]", contractIndex))


Might be worth talking to @jimthematrix about the Fabric side of this, and whether in practice we use this feature you've added for jumping chaincode names, vs. just upgrading the Fabric chaincode in-place. Not a blocker to this merging, but something to think about in the next step when developing the v.next contract code, and testing this journey end to end with a Fabric environment.

internal/events/operator_action.go

db/migrations/postgres/000090_add_namespace_contractindex.up.sql

peterbroadhurst

This is looking great @awrichar - a couple of (hopefully small) changes requested before we merge, along with a bunch of additional musings.

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

peterbroadhurst

👍 - really pleased this all worked through, and we have an upgrade path in place to adopt the new per-namespace contracts.

awrichar requested review from peterbroadhurst, nguyer and nickgaski as code owners May 17, 2022 17:39

awrichar marked this pull request as draft May 17, 2022 17:39

awrichar force-pushed the migration branch from e999cad to e101f69 Compare May 18, 2022 18:27

awrichar marked this pull request as ready for review May 18, 2022 18:29

awrichar mentioned this pull request May 19, 2022

Move all identity APIs under /namespaces #804

Merged

awrichar added 3 commits May 20, 2022 16:58

Add new Ethereum contract config

54b4724

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

Split Ethereum websocket init from other init

2ddb05d

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

Add new Fabric contract config

26fdac4

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

awrichar force-pushed the migration branch from e101f69 to 67d4802 Compare May 20, 2022 20:58

Support migration of the FireFly contract from one location to another

cd6d752

Send a specially formatted "BatchPin" transaction to signal that all members should migrate to a specific version of the contract from their config. Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

awrichar force-pushed the migration branch from 67d4802 to cd6d752 Compare May 20, 2022 21:04

peterbroadhurst reviewed May 20, 2022

View reviewed changes

internal/apiserver/route_post_network_migrate.go Outdated Show resolved Hide resolved

peterbroadhurst reviewed May 20, 2022

View reviewed changes

internal/database/sqlcommon/namespace_sql.go Outdated Show resolved Hide resolved

peterbroadhurst reviewed May 20, 2022

View reviewed changes

internal/blockchain/ethereum/ethereum.go Outdated Show resolved Hide resolved

awrichar added 5 commits May 24, 2022 12:37

Store termination info for every previous FireFly contract

6f8257f

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

Replace /network/migrate with /network/operatoraction

a0428c2

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

Test coverage and cleanup

a94a153

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

Merge remote-tracking branch 'origin/main' into migration

e75a5c2

Remove tracking of "final events" from blockchain event processing

864af2e

This is already covered by tracking of the current subscription ID, which will filter out events from unrecognized/old subscriptions. Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

awrichar commented May 24, 2022

View reviewed changes

internal/blockchain/ethereum/ethereum.go Show resolved Hide resolved

Add mutex and some extra comments

7b540f6

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

awrichar commented May 25, 2022

View reviewed changes

internal/blockchain/ethereum/ethereum.go Show resolved Hide resolved

peterbroadhurst reviewed May 26, 2022

View reviewed changes

internal/blockchain/ethereum/ethereum.go Show resolved Hide resolved

peterbroadhurst reviewed May 26, 2022

View reviewed changes

internal/blockchain/fabric/fabric.go Show resolved Hide resolved

peterbroadhurst reviewed May 26, 2022

View reviewed changes

internal/events/operator_action.go Outdated Show resolved Hide resolved

peterbroadhurst reviewed May 26, 2022

View reviewed changes

db/migrations/postgres/000090_add_namespace_contractindex.up.sql Outdated Show resolved Hide resolved

peterbroadhurst requested changes May 26, 2022

View reviewed changes

awrichar added 5 commits May 26, 2022 13:57

Use the simpler "network action" over "network operator action"

ebd9371

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

Rename database migration files

c715fab

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

Reduce the scope of mutex in ethereum/fabric contract calls

620881c

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

Validate that all network actions come from a registered root org

904cacc

Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>

Merge remote-tracking branch 'origin/main' into migration

2d08562

peterbroadhurst approved these changes May 27, 2022

View reviewed changes

peterbroadhurst merged commit 6229fed into hyperledger:main May 27, 2022

peterbroadhurst deleted the migration branch May 27, 2022 19:29

awrichar added this to the v1.1.0 milestone May 31, 2022

awrichar mentioned this pull request May 31, 2022

Update FireFly smart contract(s) to be more extensible for the future #846

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support migration of the FireFly contract from one location to another #820

Support migration of the FireFly contract from one location to another #820

awrichar commented May 17, 2022 •

edited

Loading

codecov-commenter commented May 17, 2022 •

edited

Loading

awrichar commented May 20, 2022

awrichar commented May 20, 2022

peterbroadhurst commented May 20, 2022

awrichar commented May 20, 2022

peterbroadhurst commented May 20, 2022

awrichar commented May 20, 2022 •

edited

Loading

peterbroadhurst May 26, 2022

awrichar May 26, 2022

peterbroadhurst May 26, 2022

peterbroadhurst left a comment

peterbroadhurst left a comment

Support migration of the FireFly contract from one location to another #820

Support migration of the FireFly contract from one location to another #820

Conversation

awrichar commented May 17, 2022 • edited Loading

codecov-commenter commented May 17, 2022 • edited Loading

Codecov Report

awrichar commented May 20, 2022

awrichar commented May 20, 2022

peterbroadhurst commented May 20, 2022

awrichar commented May 20, 2022

peterbroadhurst commented May 20, 2022

awrichar commented May 20, 2022 • edited Loading

peterbroadhurst May 26, 2022

Choose a reason for hiding this comment

awrichar May 26, 2022

Choose a reason for hiding this comment

peterbroadhurst May 26, 2022

Choose a reason for hiding this comment

peterbroadhurst left a comment

Choose a reason for hiding this comment

peterbroadhurst left a comment

Choose a reason for hiding this comment

awrichar commented May 17, 2022 •

edited

Loading

codecov-commenter commented May 17, 2022 •

edited

Loading

awrichar commented May 20, 2022 •

edited

Loading