Fix insertion of bn256 keys into secp_bls_mapping

[tmpolaczyk]

The get_rep_ordered_bn256_list function is used to create a merkle tree of BLS keys which is included in superblocks as the ars_root field. This field is supposed to be used as a validation in a decentralized bridge, but there is no decentralized bridge yet so it is not used.

However a quick test shows that the number of identities returned by the get_rep_ordered_bn256_list function is much smaller than the number of active identities. For example, when there are 13000 active identities only 650 are returned by that function. This is the function definition:

https://github.com/witnet/witnet-rust/blob/4007d83f5a482786b667dd7cb6c333f0baa2ccd5/data_structures/src/superblock.rs#L78-L83

Since identities are only removed if they do not have a corresponding BN256 key, it is possible that there is a bug in the way we store the BN256 keys.

[tmpolaczyk]

I found the cause. bn256 keys can be found in two places: the miner key in the block header, and the witness keys in the commit transactions. The miner keys are working as expected but the commit keys are essentially ignored. This is because identities are only marked as active after processing a data request tally, and we insert the bn256 keys from commit transactions which happen a few blocks before the tally. This means that the keys are correctly inserted into the secp_bls_mapping, but on the next block they are removed because the keys from inactive identities are always removed. A few blocks later this identities will be active, but the keys have already been deleted.

I used this branch to test it in a local testnet. Setup:

• Start 2 nodes with mining=true
• Wait until both nodes have mined a block, and start 1 node with mining=false
• Send some wits to the third node to make it able to resolve data requests
• Launch data request with 3 witnesses

Using the logs I made this table to compare the length of the ARS with the length of the secp_bls_mapping. Each Bn is one block: B1 has the data request, B2 has the commits, B3 has the reveals, B4 the tally. The two columns should be equal on every "Add new keys" step.

Event	ARS len	secp_bls_mapping len
B1 Remove inactive	2	2
B1 Add new keys	2	2
B2 Remove inactive	2	2
B2 Add new keys	2	3
B3 Remove inactive	2	2
B3 Add new keys	2	2
B4 Remove inactive	3	2
B4 Add new keys	3	2
B5 Remove inactive	3	2
B5 Add new keys	3	2

You can see how "B2 Add new keys" is adding the new key from the commit transaction of node 3, but this key is removed in the next block. Then in block B4 the ARS is updated using the tally of the data request, and the secp_bls_mapping is now missing one key.

And even though now node 3 is active, if we send another data request it will not send the bn256 key. That's because there is an optimization that makes active nodes not send their bn256 keys, to avoid sending duplicated data (they must have been inactive at some point, so that's when they should have sent their bn256 key). Similarly, if node 3 mines a block while being active it will not insert the bn256 key into the block header. We could remove this optimization to increase the amount of keys in secp_bls_mapping (this would NOT be a breaking change) but I don't think it's worth it because this bn256 keys are not used anywhere yet.

[tmpolaczyk]

Possible solutions

I already tried one solution in the test branch, but it changes the ARS logic so it is a hard fork. This solution is to insert all the public keys from commit transactions into the ARS, this way the identities are marked as active a few blocks before. This would be the table:

Event	ARS len	secp_bls_mapping len
B1 Remove inactive	2	2
B1 Add new keys	2	2
B2 Remove inactive	3	2
B2 Add new keys	3	3
B3 Remove inactive	3	3
B3 Add new keys	3	3
B4 Remove inactive	3	3
B4 Add new keys	3	3
B5 Remove inactive	3	3
B5 Add new keys	3	3

Another solution is to do the opposite, and delay the insertion into secp_bls_mapping until the tally. This is also a hard fork because it changes the secp_bls_mapping, which affects the superblock hash. This would be the table:

Event	ARS len	secp_bls_mapping len
B1 Remove inactive	2	2
B1 Add new keys	2	2
B2 Remove inactive	2	2
B2 Add new keys	2	2
B3 Remove inactive	2	2
B3 Add new keys	2	2
B4 Remove inactive	3	2
B4 Add new keys	3	3
B5 Remove inactive	3	3
B5 Add new keys	3	3

So, since this will be a hard fork I think we should wait until the design of the desentralized bridge is finalized, and then make all the breaking changes at once. For example we may want to change the bn256 curve into a different one.