After changing group_id treasurer stops working

[astroseger]

Way to reproduce the bug:

1. Service is running and receiving payments.
2. Service provider update metadata and intentionally or by mistake change group_id. It should be noted that it is strongly not recommended, and in the last version of snet-cli it is not possible by default (however it still can be done with --force option). In order to change group_id your could simple metadata-init which will reinitialize group_id, and after you update metadata in Registry by snet service update-metadata (but in the last version you will need to use force here).
3. Restart daemon
4. open new payment channel and make few calls from the client side.
5. Run snet treasurer claim-all, and it will fail with "Channel received belongs to another group of replicas,...." error from the daemon...

The possible cause of the problem is following: then daemon execute treasurer related commands it sometime verify that group_id of the channel corresponds to the current group_id and sometimes not...:

• GetListUnclaimed return list of all payment channels (including channels with old group_id)
• StartClaim works only with channels with new group_id

I would suggest that in controle_service daemon should work with all channels with correct payment_address. The owner of payment_address (and we verify it with the signature) has right to do anything with his channels. So we shouldn't verify that group_id is still correct..
(even though the changing of group_id is not recommended)

[anandrgitnirman]

hi @astroseger with the latest change in snet-cli from what I understand it is not possible to modify an existing group_id right in metadata ?

[astroseger]

Yes. service provider should never do it. If your really need it you can force it by removing and adding service again. But all payment channels will became invalid, and we still have a bug in daemon #224, so your daemon will also stop working.

[astroseger]

@anandrgitnirman
Yes it is impossible to do it via snet-cli. But still it is bug here. Service provider should be able to claim their funds even in case of changing group-id.

[anandrgitnirman]

@vsbogd , @astroseger

We have the approaches to bypass the group ID check at the time of Claim
1)
Introduce a new attribute DisableGroupIdCheck bool under PaymentChannelKey

snet-daemon/escrow/payment_channel_api.go

Lines 40 to 42 in e9ff650

	type PaymentChannelKey struct {
	ID *big.Int
	}

then check for this flag at

snet-daemon/escrow/payment_channel_storage.go

Line 130 in e9ff650

if ch.GroupId != configGroupID {

We can re name PaymentChannelKey to PaymentChannelKeyOptions

Introduce a new param in the 2 core API methods to take an additional parameter

snet-daemon/escrow/payment_channel_api.go

Line 111 in 6005fdf

PaymentChannel(key *PaymentChannelKey) (channel *PaymentChannelData, ok bool, err error)

snet-daemon/escrow/payment_channel_api.go

Line 126 in 6005fdf

PaymentChannelFromBlockChain(key *PaymentChannelKey) (channel *PaymentChannelData, ok bool, err error)

Move the functionality on recipient check and Group ID checks to separate methods out of the method GetChannelStateFromBlockchain ( ideally this method should just read from Block chain based on the description ?)

snet-daemon/escrow/payment_channel_storage.go

Lines 118 to 154 in e9ff650

	func (reader *BlockchainChannelReader) GetChannelStateFromBlockchain(key *PaymentChannelKey) (channel *PaymentChannelData, ok bool, err error) {
	ch, ok, err := reader.readChannelFromBlockchain(key.ID)
	if err != nil || !ok {
	return
	}
	configGroupID, err := reader.replicaGroupID()
	if err != nil {
	return nil, false, err
	}
	recipientPaymentAddress := reader.recipientPaymentAddress()
	if ch.GroupId != configGroupID {
	log.WithField("configGroupId", configGroupID).Warn("Channel received belongs to another group of replicas")
	return nil, false, fmt.Errorf("Channel received belongs to another group of replicas, current group: %v, channel group: %v", configGroupID, ch.GroupId)
	}
	if recipientPaymentAddress != ch.Recipient {
	log.WithField("recipientPaymentAddress", recipientPaymentAddress).
	WithField("ch.Recipient", ch.Recipient).
	Warn("Recipient Address from service metadata not Match on what was retrieved from Channel")
	return nil, false, fmt.Errorf("recipient Address from service metadata does not Match on what was retrieved from Channel")
	}
	return &PaymentChannelData{
	ChannelID: key.ID,
	Nonce: ch.Nonce,
	State: Open,
	Sender: ch.Sender,
	Recipient: ch.Recipient,
	GroupID: ch.GroupId,
	FullAmount: ch.Value,
	Expiration: ch.Expiration,
	Signer: ch.Signer,
	AuthorizedAmount: big.NewInt(0),
	Signature: nil,
	}, true, nil
	}

Ignore the Recipient check when deliberately

[vsbogd]

@anandrgitnirman, I think we need to have a clear description why snet treasurer claim-all fails after changing group_id. If you have one could you please post it here.

[anandrgitnirman]

Hi @vsbogd

1. When the Daemon starts ( group ID from the meta data is set at the below )
   

   snet-daemon/escrow/payment_channel_storage.go

   Line 104 in 6005fdf

   s := metadata.GetDaemonGroupID()

2. the Below two methods are called during claim
   PaymentChannel
   GetChannelStateFromBlockchain

the group ID on step 1 and 2 are different thus throwing back an error as pointed out by Sergey .

[vsbogd]

Thanks @anandrgitnirman. I would like to explain some group and groupId background before suggesting the solution.

Group of daemons (or service replicas) is some set of daemons which use the same etcd storage to keep off-chain channel state. It means that if some service is deployed using two groups of replicas (A and B) then daemons in group A doesn't have access to off-chain channel state for channels used by daemons in group B. It is the only reason why daemon groups exist. Typical use case is having groups of daemons in different geographical locations like UK and Singapoor. Then it is too slow to have shared etcd storage for them and we can introduce two local storages and two groups of daemons.

GroupId is required to forbid using payment channels of group A by daemons from group B. This restriction is required because daemons in group B doesn't know off-chain state for channels of group A (see previous paragraph). And this is the reason why they cannot use channels of group A correctly.

[vsbogd]

Returning to the issue use case. Service publisher published service using groupId X and after some time reconfigured service and changed groupId to Y. While daemon storage is not cleaned up it remembers off-chain state of the group X. The only reason it cannot manipulate with it is that blockchain reader has the guard: to not read channel state for channel of another group. I think it is incorrect guard then.

In other words I would propose to remove following check from BlockchainChannelReader:

	configGroupID, err := reader.replicaGroupID()
	if err != nil {
		return nil, false, err
	}
	recipientPaymentAddress := reader.recipientPaymentAddress()

	if ch.GroupId != configGroupID {
		log.WithField("configGroupId", configGroupID).Warn("Channel received belongs to another group of replicas")
		return nil, false, fmt.Errorf("Channel received belongs to another group of replicas, current group: %v, channel group: %v", configGroupID, ch.GroupId)
	}

and move it to lockingPaymentChannelService.PaymentChannel() method to check groupId only when channel is not found in storage. As result groupId check will be applied only first time when channel is added to storage from the blockchain. If channel is already in storage then groupId check will be skipped.

[anandrgitnirman]

fixed released on Daemon, closing this issue