In LIP-45. The constant MIN_RETURN_FEE_PER_BYTE_LSK is used to represent minimum return fee per byte. The suffix _LSK would lead to confusion since the value is actually in Beddows.

Suggested name would be MIN_RETURN_FEE_PER_BYTE_BEDDOWS

https://github.com/LiskHQ/lips/blob/main/proposals/lip-0045.md#sidechain

If chainInfos is not empty, then check that:
ownChainName is from the character set a-z0-9!@$&_., has length between MIN_CHAIN_NAME_LENGTH and MAX_CHAIN_NAME_LENGTH, and ownChainName != CHAIN_NAME_MAINCHAIN;

Here it needs to be in this order

If chainInfos is not empty, then check that:
ownChainName has length between MIN_CHAIN_NAME_LENGTH and MAX_CHAIN_NAME_LENGTH, is from the character set a-z0-9!@$&_., and ownChainName != CHAIN_NAME_MAINCHAIN;

Because if we provide ownChainName with an empty string "", regex check will fail.
Expected behaviour was to check against MIN_CHAIN_NAME_LENGTH, but in reality regex check was performed.

Input type of createAggSig in LIP 0038 is wrong (should be an object, or the pseudocode should be modified).

The params property of the unlock command is expected to be empty bytes, but it is never verified. Therefore, someone could create an unlock transaction in which the params property is not empty and the transaction is accepted.

Change the name of the endpoint verifyBLSKey to validateBLSKey as it was changed in the implementation.

Description

Introduce the possibility that in the Fee module, fees are transferred to a specified account instead of burning them. This could be achieved by:

• Adding configurable constant BURN_FEES (boolean) and ADDRESS_FEE_POOL (20 byte address), the names are up to discussion here.
• In LIP 48 replace

Token.burn(address, tokenID, amount),

by

if BURN_FEES:
    Token.burn(address, tokenID, amount)
else:
    Token.transfer(address, ADDRESS_FEE_POOL, tokenID, amount)

Motivation

For sidechains, it can be beneficial to have the options to collect fees in a pool (in particular, when using LSK for fees) and use them to incentivize validators or other protocol participants.

Note that an issue with the same suggestion was also created in the SDK repository: LiskHQ/lisk-sdk#8028.

Currently, beforeCrossChainMessageForwarding of token module (LIP 51) erroneously assumes that an escrow account (ccm.receivingChainID, messageFeeTokenID) exists. This is implicit, since there is an attempt to update escrow accounts as follows:

escrowAmount(ccm.receivingChainID, messageFeeTokenID) += ccm.fee

So in case an escrow account for ccm.fee for the receiving chain is not initialized yet (this can happen for example if the fowarded ccm is the first ccm to be sent to this chain after it became active in the mainchain), then this fails and as a result the sending chain gets terminated (see LIP49 for handling errors in beforeCrossChainMessageForwarding)

Suggested Solution

We will initialize an escrow account for the message fee token during chain registration (LIP 43). This will make sure that for all cases, an escrow account for the token used in ccm.fee is initialized. This also simplifies the payMessageFee function of the token module, which currently checks if an escrow account is initialized for the message fee token.

In LIP 0045, it is specified that "the own chain account is present by default, set to an object with properties: ...". This can, however, not be implemented in the interoperability module in general and can also not be specified in the "Genesis Block Processing" section of LIP 0045 as this a rule specific to the mainchain and does not hold in general.
Therefore, it must be specified in LIP 0063.

The line outboxKey = MODULE_NAME_INTEROPERABILITY + SUBSTORE_PREFIX_OUTBOX_ROOT + sha256(partnerchainID)
in verifyPartnerChainOutboxRoot should be outboxKey = STORE_PREFIX_INTEROPERABILITY + SUBSTORE_PREFIX_OUTBOX_ROOT + sha256(OWN_CHAIN_ID)

The link pointing to https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki is broken.

https://github.com/LiskHQ/lips/blob/main/proposals/lip-0049.md#verification-1

if chainAccount(ccm.sendingChainID).status != CHAIN_STATUS_REGISTERED:
        raise Exception("Registration message must be sent from a chain with status 'registered'.")

As per https://github.com/LiskHQ/lips/blob/main/proposals/lip-0045.md#notation-and-constants
CHAIN_STATUS_REGISTERED has integral value 0

In above error message, 'registered' should be replaced with it's integral value 0.
Because if we pass/provide registered as hard coded string value in some test fixture, above If condition will fail and it will lead to confusion. User will think that I've passed correct value as per error message, but in reality comparison was made with integral value of CHAIN_STATUS_REGISTERED

Align LIP 0063 with the changes in this PR: #256. Namely, update the addInteropModuleEntry function.

message recovery intialization

queryKey = STORE_PREFIX_INTEROPERABILITY + SUBSTORE_PREFIX_CHANNEL_DATA + sha256(getMainchainID())

should

queryKey = STORE_PREFIX_INTEROPERABILITY + SUBSTORE_PREFIX_CHANNEL_DATA + sha256(OWN_CHAIN_ID)

State recovery

query = {
  "key": storePrefix + entry.substorePrefix + entry.storeKey,
  "value": sha256(entry.storeValue),
  "bitmap": entry.bitmap
}

should be

query = {
  "key": storePrefix + entry.substorePrefix + sha256(entry.storeKey),
  "value": sha256(entry.storeValue),
  "bitmap": entry.bitmap
}

Solve the problems related to using LSK as default token for fees in a sidechain, e.g:
Is it actually possible to use LSK as default token for fees in a sidechain? To bring LSK tokens to the sidechain to begin with you have in general to pay the transaction fee of the CCU.

{
	type: 'array',
	items: {
		type: 'object',
		required: ['blsKey', 'bftWeight'],
		properties: {
			blsKey: {
				dataType: 'bytes',
				fieldNumber: 1,
				minLength: BLS_PUBLIC_KEY_LENGTH,
				maxLength: BLS_PUBLIC_KEY_LENGTH,
			},
			bftWeight: {
				dataType: 'uint64',
				fieldNumber: 2,
			},
		},
	},
	minItems: 1,
	maxItems: MAX_NUM_VALIDATORS,
}

Currently we have this schema object, which is repeated/copy pasted multiple times: Some examples:

• mainchainRegParams
• sidechainRegParams
• validatorsHashInputSchema
• registrationSignatureMessageSchema
• validatorsDataSchema

What we can do alternatively, is to define this schema object once & then use/inject wherever needed.

// declaration
const chainValidators = {
	type: 'array',
	// Ignore/skip `fieldNumber` here
	items: {
		type: 'object',
		required: ['blsKey', 'bftWeight'],
		properties: {
			blsKey: {
				dataType: 'bytes',
				fieldNumber: 1,
				minLength: BLS_PUBLIC_KEY_LENGTH,
				maxLength: BLS_PUBLIC_KEY_LENGTH,
			},
			bftWeight: {
				dataType: 'uint64',
				fieldNumber: 2,
			},
		},
	},
	minItems: 1,
	maxItems: MAX_NUM_VALIDATORS,
};

// usage
export const sidechainRegParams = {
	$id: '/modules/interoperability/mainchain/sidechainRegistration',
	type: 'object',
	required: ['chainID', 'name', 'sidechainValidators', 'sidechainCertificateThreshold'],
	properties: {
		...
		sidechainValidators: {
			...chainValidators, // inject
			fieldNumber: 3, // & specify correct fieldNumber here
		},
		...
	},
};

Reasons/Motivation

• It will avoid human error if certain checks like minItems|maxItems & minLength|maxLength is forgotten to apply somewhere
• We can control this validators schema from one central place
• We wouldn't need to repeat/copy paste every time

https://github.com/LiskHQ/lips/blob/main/proposals/lip-0061.md#schema

unsignedCertificateSchema = {
    "type": "object",
    "required": [
        "blockID",
        "height",
        "timestamp",
        "stateRoot",
        "validatorsHash"
    ],
    "properties": {
        "blockID": {
            "dataType": "bytes",
            "length": HASH_LENGTH,
            "fieldNumber": 1
        },
        "height": {
            "dataType": "uint32",
            "fieldNumber": 2
        },
        "timestamp": {
            "dataType": "uint32",
            "fieldNumber": 3
        },
        "stateRoot": {
            "dataType": "bytes",
            "length": HASH_LENGTH,
            "fieldNumber": 4
        },
        "validatorsHash": {
            "dataType": "bytes",
            "length": HASH_LENGTH,
            "fieldNumber": 5
        }
    }
}

certificateSchema = {
    "type": "object",
    "required": [
        "blockID",
        "height",
        "timestamp",
        "stateRoot",
        "validatorsHash",
        "aggregationBits",
        "signature"
    ],
    "properties": {
        "blockID": {
            "dataType": "bytes",
            "length": HASH_LENGTH,
            "fieldNumber": 1
        },
        "height": {
            "dataType": "uint32",
            "fieldNumber": 2
        },
        "timestamp": {
            "dataType": "uint32",
            "fieldNumber": 3
        },
        "stateRoot": {
            "dataType": "bytes",
            "length": HASH_LENGTH,
            "fieldNumber": 4
        },
        "validatorsHash": {
            "dataType": "bytes",
            "length": HASH_LENGTH,
            "fieldNumber": 5
        },
        "aggregationBits": {
            "dataType": "bytes",
            "fieldNumber": 6
        },
        "signature": {
            "dataType": "bytes",
            "length": BLS_SIGNATURE_LENGTH,
            "fieldNumber": 7
        }
    }
}

Just like in LIP 45, we used ... notation to merge external schema, we can do the same here to merge repeated properties from unsignedCertificateSchema

One example:
https://github.com/LiskHQ/lips/blob/main/proposals/lip-0045.md#data-3

ccmProcessedDataSchema = {
    "type": "object",
    "required": ["ccm", "result", "code"],
    "properties": {
        "ccm": {
            ...crossChainMessageSchema,
            "fieldNumber": 1
        },
        "result": {
            "dataType": "uint32",
            "fieldNumber": 2
        },
        "code": {
            "dataType": "uint32",
            "fieldNumber": 3
        }
    }
}

Also we can add this text below updated schema:

Here, the ... notation, borrowed from JavaScript ES6 data destructuring, indicates that the corresponding schema should be inserted in place, and it is just used for notational convenience.

Note: Above text is taken from https://github.com/LiskHQ/lips/blob/main/proposals/lip-0045.md#genesis-assets-schema, which is written under the schema.

if aggregationBits[i // 8] & (1 << (i % 8)) == 1
Should rather be:
if (aggregationBits[i // 8] >> (i % 8)) & 1 and it matches with the implementation. So, we just have to change in the LIP.

In the validation of incoming single commits, we currently do not consider the case that the node validating the single commits may be lagging behind compared to the node signing the single commit. Such a single commit m will satisfy

m.height > getBFTHeights().maxHeightPrecommitted

The current approach should work and not lead to accidental banning as step 1-4 should ensure that also step 5 and 6 pass (concretely check 4 will ensure this.). However, in step 3 we discard single commits satisfying the property above in many instances. It would be cleaner to either

1. Always discard single commits satisfying the property above.
2. Always keep single commits satisfying the property above (if they pass the other checks, in particular, 4-6).

Approach 2 seems a bit more robust as single commits are not discarded just because a node lags a bit behind at the moment.

The statement "This command cannot be used to modify an existing BLS key, as this is forbidden by the Validators module" is outdated because the Validators module does allow it.

1. The interfaces of the execute and verify functions of cross chain commands require that the first argument is of type Transaction. For example, the interface of the verify function is in all existing cases def verify(trs: Transaction, ccm: CCM) -> None:. But they are called with arguments of type CCU here, here and here.
2. The function verifyCrossChainMessage in the Token module LIP also expects an object of type Transaction as the first argument, but when called from LIP 0049 (here and here), an object of type CCU is provided.

Both ways, either changing the interface or changing the calls, should be fine.

The property name in ownChainAccountSchema has minLength, however in genesis assets this property (ownChainName) does not have minimal length. Add "minLength": MIN_CHAIN_NAME_LENGTH to the genesis assets schema in LIP 0045.

LIP 0041 uses the function name beforeCommandExecution, although it should be beforeCommandExecute as defined in LIP 0069.

The property accountInitializationFee is outdated and should be removed from the specification of the LIP 0068.

Currently in the PoS module (LIP 57) there are the following logic mistakes in the validator selection:

• The unbanValidator function does not include the unbanned validator in the eligible delegates even if they meet the requirements.
• getActiveValidators function has some flaws for the case of "hybrid" rounds (i.e., the rounds between bootstrap period and full PoS period): First it implicitly assumes that there are always nbrElectedValidators eligible validators which might not be the case. If this is not the case the average bft weight of elected validators needs to be calculated differently. Also it has to account for the case where nbrElectedValidators = 0.

LIP 0057 uses the following condition in the genesis block processing:

# For the snapshot block for the migration from Lisk Core 3 to Lisk Core 4 on
# Lisk mainnet, apply a special rule. This must be done as validators in
# this snapshot block do not have a BLS key.
if the chain is the mainchain and b is the snapshot block for the migration:

The 2nd part of the condition (b is the snapshot block for the migration) can however not be detected by the SDK. And adding the migration height as a config parameter is not seen as nice solution to the SDK team. Instead, a boolean config value is used that indicates if (A) all validators do not have a BLS key or (B) all validators have a valid BLS key. Case (A) will be used for the re-genesis block for Lisk mainchain v4. Case (B) for all other (re)-genesis blocks. LIP 0057 must be fixed such that this config value is used in the if statement instead of "is the snapshot block for the migration".
Note that the condition if the chain is the mainchain MUST still be checked to prevent that any other chain calls registerValidatorWithoutBLSKey instead of registerValidatorKeys.

Possibly, this config value must also be specified in LIP 0063. Further clarification is needed.

At the length of the following properties is not validated:

• blockID
• stateRoot
• validatorsHash
• signature

The length validation of these properties should be added. In particular, this ensures that certificates included in CCUs have the desired format.

LIPs 42, 71 (reward/dynamic reward):

Functions getBlockReward/getDynamicBlockReward Use the argument blockHeader.seedReveal , which does not exist according to the new block schema. Instead this property is part of the block assets; therefore the block assets should also be arguments of this function. Fixing this perhaps requires updating the isSeedRevealValid function of the random module: the second argument should be the block assets and the de-serialization should probably happen inside this function.

Random module (LIP 46):

• Inconsistent type hinting: function H() has as input bytes. However in getRandomBytes() it is called with uint32 as input. Need to transform int to bytes in the call of H().
• AfterTransactionsExecution is described in an inconsistent way: It checks if "size of the validatorReveals array is larger than MAX_LENGTH_VALIDATOR_REVEALS" and if yes, it removes the last element. It should be "if size of array == MAX_LENGTH_VALIDATOR_REVEALS. Also the description about MAX_LENGTH_VALIDATOR_REVEALS could be moved to the table.

The 'After Block Execution' header in LIP 0042 should be `After Transactions Execution'.

Instead of h1 = getMaxRemovalHeight(), we should use h1 = maxHeightCertified.
See also https://github.com/LiskHQ/lisk-sdk/pull/8095/files#r1091715130.

In LIP 0044, there is a SUBSTORE_PREFIX_GENESIS_DATA substore prefix defined. However it is not used in the LIP.
I think, this constant should be removed from the LIP.

The genesis asset schema in LIP 0045 duplicates a lot of state store information and requires many self-consistency checks. It should be refactored in style of LIP 0057.

• Improve the pseudocode to align with python style.
• Update rationale to describe node batching
• implement multiproof algorithm improvements.
• Add check for bitmap extra 0s in verify
• Add Lisk-specific hash prefixes: LiskHQ/lisk-db#117
• Specify verifyInclusionProof API: LiskHQ/lisk-db#116
• Add check that all sibling hashes are consumed in verify: LiskHQ/lisk-db#120

• The endpoint isChainIDAvailable should be moved to a new endpoint section (currently missing).
• The logic should return False if the input is not on the same network of the mainchain, or if the input equals the mainchain chainID.
• We should add a similar endpoint for registered names isChainNameAvailable. This should check if the name has the required length, uses the correct character set, and if it is available.

Related SDK issues:

• LiskHQ/lisk-sdk#8190
• LiskHQ/lisk-sdk#8191

For the recovery in LIP 54, we define the following two functions

• applyRecovery(trs: Transaction, ccm: CCM)
• forwardRecovery(trs: Transaction, ccm: CCM)

which are the analog of apply and forward defined in LIP 0049. The difference though being that for hook calls such as mdl.beforeCrossChainCommandExecution(trs, ccm) the first argument is not a CCU transaction any more, but instead a message recovery transaction for instance. I am not sure this always works because in some of these hooks in different modules we rely on the fact that trs.params follows the CCU parameter schema and we use specific CCU parameter properties.

Emit a persistent event during the CCU execution ifverifyCertificateSignature fails (similar to what is done in LIP0043 for the registration message).

Related comment: #255 (comment)

When calling verifyLivenessConditionForRegisteredChains, the certificate bytes should have already been validated against the certificate schema.

In this line, the engine calls the validators module to get some data (generatorKey). This is, however, not possible with the current architecture. Moreover, the implementation differs from the LIP in this aspect: it gets the data from the BFT store. The LIP should be updated to follow the implementation in this regard.

The type Transaction is used in several places, but it is not defined. In particular, it is not clear that for an instance of Transaction the params property is of type object instead of bytes.

The change depends on the conclusions of Improve LIP template discussion.

In the function setBFTParameters we do not check whether the provided addresses, generator keys and BLS keys in the parameter validatorList are unique. This is guaranteed by the application layer at the moment (i.e., PoA, PoS and Validators module), but for more robustness it would be better to add this check in the engine. With duplicate addresses and BLS keys the behavior of the engine is not well-defined any more and it would be best to raise an exception explicitly.

In the SDK, during the Command verification hook for a transaction, a schema validation of the params property is performed. This is currently not mentioned in the specifications. Moreover, the encoding of the params is not explicitly defined in LIPs at the moment. The same holds for the params property of CCMs.

It is possible for sidechains that, while assigning rewards to block generator (either block rewards or unburned transaction fees), a user account for the corresponding token (reward/ fee token) does not exist for the generator, leading to this generator being unable to add blocks.

Description

While writing the guide "How to register a sidechain", I wanted to explain to the reader, how to check the status of their sidechain by getting the sidechain account.
I was surprised to not find any existing endpoints for this.
As it turns out, this functionality is currently not built-in in the Interoperability module and it's related LIP 0045.

From user and docs perspective, I consider such an endpoint quite relevant - how else can they check about the status of a sidechain?

It would be also great if it could be reconsidered as part of this issue to add other similar endpoints to the module, too, which provide relevant data to the users. Please try to look at it from a users perspective.
E.g. a list of all chain IDs of sidechains that are registered on the Mainchain would be interesting as well, imho.

Ensure that a sidechain following the standard Interoperability protocol, i.e. implementing the Interoperability and Token modules coming from the Lisk SDK, cannot terminate the mainchain.

Description

For this PR, there were some points which are difficult to implement

• https://github.com/LiskHQ/lips/pull/192/files#diff-851f39ba2dc407b594e12baee157421d0efd44abd262b8d24cd1b7d20ae0dd8fR2522-R2523
  Here, it's defined for all chain, but it's only called in the mainchain
• https://github.com/LiskHQ/lips/pull/192/files#diff-851f39ba2dc407b594e12baee157421d0efd44abd262b8d24cd1b7d20ae0dd8fR1855-R1858
  At this point, it is not possible to fetch what modules are registered. We could check if dependency is resolved or not, but if checked here, it should checked everywhere

Also,
on transferCrossChain method, check for the mainchain ID should be removed

LIP 0068 points to the outdated definition of signEd25519 from LIP 0037 instead of from LIP 0062 in this line.

• replace blockHeader.impliesMaximalPrevotes -> blockHeader.impliesMaxPrevotes in LIP 71
• replace impliesMaximalPrevotes -> impliesMaxPrevotes in LIP 58. The sentence "Functions that allow to verify the Lisk-BFT related properties in the block header, namely validatorsHash, maxHeightGenerated, maxHeightPrevoted and impliesMaximalPrevotes" has the typo regarding block header property.

In LIP 0048, the function names

• beforeCrossChainCommandExecute
• afterCrossChainCommandExecute

are used, although the names should be

• beforeCrossChainCommandExecution
• afterCrossChainCommandExecution

as specified in LIP 0049.

In current PoS module rounds can be classified into three catagories:

• Initial rounds: The first initRounds of the blockchain. During those rounds blocks are generated by the validators specified in genesis block, called initValidators. All initValidators have BFT weight 1.
• Intermediate Rounds: After initial rounds end, the next NUMBER_ACTIVE_VALIDATORS rounds are intermediate rounds where part of blocks are generated by validators elected from staking and other blocks from initValidators. In the ith intermediate round, the top i validators from staking become active validators and the remaining NUMBER_ACTIVE_VALIDATORS - i are selected from initValidators. All validators have BFT weight 1.
• PoS Rounds: Now all active delegates are selected according to our PoS mechanism and their BFT weight is proportional to their stake.

The transition of BFT weight between "Intermediate rounds" and "PoS rounds" is very abrupt and weakens the security guarantees of our consensus. To remedy this we propose the following solution:

• During intermediate rounds, the BFT weight is distributed between elected and initial validators proportional to their participation. Therefore in the ith intermediate round, i/NUMBER_ACTIVE_VALIDATORS fraction of total BFT weight goes to elected validators and the remaining fraction goes to the initial validators. To achieve this, we will first calculate the BFT weight of elected validators according to their stake and then set the BFT weights of each initial validator equal to the average BFT weight of elected validators. After this BFT-weight assignment is in-place, capping will be applied in the same way as in the PoS rounds case.

LIP 0043

Fix mainchainRegParams schema, missing mainchainCertificateThreshold in required properties and switch ownChainID and ownName for symmetry.

LIP 0045

• In bounce function, minFee for CCMs assumes that the tokenID is LSK. We could remove the requirement if a direct channel exists instead.

• In genesis block initialization, ensure that for the mainchain the default entries are present (mainchain own chain account and registered name entry).

LIP 0053

• In verifyValidatorsUpdate function

if bftWeightsUpdate[idx].bftWeight == 0:
  raise Exception("New validators must have a positive BFT weight.")

should be replaced by

if bftWeightsUpdate[idx] == 0:

• Non-empty outboxRootWitness should be made more tight by requiring both values to be non-default.

Fix the headers of https://github.com/LiskHQ/lips/blob/main/proposals/lip-0042.md#topics. Same in LIP71