zhiqiangxu
commented
2 years ago This introduces a race condition since the list of allowed addresses is now updated every minute based on when each node first boots. As nodes aren't guaranteed to have been started at the same time, and time is never perfectly accurate, nodes will disagree on the validity of certain transactions if the sender in question was only recently authorized.
If this is instead updated every 60 blocks, at the start of each block (or for every TX in the block, I guess?), it'll execute roughly once a minute (or multiple times on the minute if executed per TX) while not risking a net split. The map in question, which is a pointer, can also be directly edited as needed by the governance module (the most efficient option, yet also one that will take a decent while longer to code than modifying what's in front of us to be based on height). The governance module would append/remove entries + save to the DB, so the only DB read is at boot for the most recently set entries.
The HTTP code to verify the relayer is valid should be removed in favor of the txnpool actor's check. It's solely redundant code right now.
ACK to the Eth block number fix. I haven't tested any of this code but except for the race condition, it seems solid.
Thanks for pointing out the redundant code, already removed.
As for updating permittedAddrMap every 1 minute, it's in order to keep the code tiny and constrained. The original code reads from database each time, so I think the current code is already enough for performance. In real world situation, it's highly unlikely that a new relayer approved 1 minute ago will already start sending tx.
This introduces a race condition since the list of allowed addresses is now updated every minute based on when each node first boots. As nodes aren't guaranteed to have been started at the same time, and time is never perfectly accurate, nodes will disagree on the validity of certain transactions if the sender in question was only recently authorized.
If this is instead updated every 60 blocks, at the start of each block (or for every TX in the block, I guess?), it'll execute roughly once a minute (or multiple times on the minute if executed per TX) while not risking a net split. The map in question, which is a pointer, can also be directly edited as needed by the governance module (the most efficient option, yet also one that will take a decent while longer to code than modifying what's in front of us to be based on height). The governance module would append/remove entries + save to the DB, so the only DB read is at boot for the most recently set entries.
The HTTP code to verify the relayer is valid should be removed in favor of the txnpool actor's check. It's solely redundant code right now.
ACK to the Eth block number fix. I haven't tested any of this code but except for the race condition, it seems solid.