Info

Download ZIP (205.6 KB)

Testing and Issues

You can test this entry and submit issues during the testing period of the TON Trustless Bridge Challenge contest.

Entries with serious issues will not be able to win the contest, but even minor issues might be important for overall results.

Voting

15
by rating

Issues

https://contest.com/docs/TrustlessBridgeChallenge
According to the task: `transaction can be a pruned branch cell.`
But you use
```
int transaction_hash = transaction.cell_hash();
;; scan for `transaction_hash` in block -> extra -> account_blocksint found? = extra.block_extra::scan_tx(transaction_hash);
```
You should use opcode `0 CHASHI`.
1
Keen Fox Feb 8 at 03:24
Agree, would be better. Not sure if it's required though.
Looks OK to me. I like the `used` bitmap for duplicate signature checking.

- It feels unnecessary to take (untrusted) root_hash and seqno as arguments in both contracts, since they're being extracted from the to-be-validated block anyway
- Might want to check for global_id in case another network uses the same validators
Keen Fox Feb 8 at 03:22
Thank you for the feedback and kind words!

Considering addition `root_hash` and `seqno` in the parameters, you are right, they are redundant but not completely useless. The intended purpose was to introduce some kind of fast and cheap "data integrity" checks. So if there is some inconsistency I could detect it early, before starting gas-intensive signatures checking.

And about another network with the same validators' keypairs (and keyblock seqno?)... Yeah) Looks very unlikely to me.
Keen Fox Feb 8 at 02:53
Thank you for the feedback! Much appreciated.

In the original paper it states "at least two-thirds" (as I cited in the comment) so I followed this precisely. But you are right that actual validators use different predicate.
In `check_block` function you check:
` throw_unless(0xbad1, prev_key_block_seqno == state::validators_source);`

But it's better to check
- `prev_key_block_seqno from testing block` >= `first key block seqno in epoch`
- `prev_key_block_seqno from testing block` <= `last known key block seqno`
Keen Fox Feb 8 at 02:54
Could you please elaborate more in what sense it is "better"?
Problem with gas usage.
In function `check_transaction` of Transaction-checker contract you go though all not pruned cells in transaction dictionary.
In functions `~shard_account_blocks::hashmap_aug::scan_tx` and `~account_block___transactions::hashmap_aug::scan_tx` you do not use `label` and remove it from the target key. In each fork of hashmap_aug you load left and right nodes. But it's pretty expensive operation. Opcode "XCTOS" consumes 126 units of gas.
Keen Fox Feb 8 at 02:58
You are right in the sense that I scan a significant part of the dictionary (at least until required tx is found, the whole in the worst case). I didn't quite get how the key is used in `ShardAccounts` structure. But I followed this logic:

1. proof is a pruned cell, so in practice it should only contain a single entry with the transaction we're interested in
2. a sender of the request is interested to comply with this rule, since gas payment is on him

Nevertheless this approach almost always scans some unnecessary cells, and could be improved no doubt.
You have already written in your code:
"TODO: potential DoS-vector: sending many request with not enough gas, that fulfill pending queries pool"
But it can be easy solved by checking msg.value. Each contract should check that msg.value > 1 ton + epsilon. And even if contract failed with out of gas than bounced message will be generated.
Proper implementation of lite-client, in tx-checker scan of all (unprunned) txses in the block (expensive but works for small blocks). Minor: no global_id checks.
Nobody added any issues yet...