The two SHA256 sums printed above from a) the downloaded CDN image and b) the locally built image, must be identical, and must match the SHA256 from the payload of the NNS proposal.
At the time of this comment on the forum there are still 2 days left in the voting period, which means there is still plenty of time for others to review the proposal and vote independently.
Hey @sat there may be several improvement opportunities that were identified by the CodeGov team. We adopted both proposals, but there is at least one item that we want to bring to your attention could be an important issue worth escalating. We recommend that DFINITY assess further and consider rejecting these proposals. One of our reviewers, @cyberowl, REJECTED proposal 125342 because of an error he found in the code of a consensus update that he felt could be a significant issue. There were several other issues identified with this particular commit by several CodeGov reviewers.
Please see the list below for all improvement opportunities identified by the CodeGov team.
We also noticed that DFINITY picked up several if the issues we identified last week and corrected them this week. That was nice to see.
It worries me that block_proposalbin is wrong. It should be block_proposal.bin . I don’t know the implications but need to be extra harsh because it is under consensus topic.
The backup helper now passes --ignore-existing to rsync command. This skips updating files on the receiver. Not clear how this relates to removing hashes from artifact names.
each artifact at a given height is stored in a bin file named with only the artifact specific name. However, they did not update the documentation of the file_location function accordingly.
This appears to be identical to c2e5826a7 from last week, no idea why this commit has been reverted twice. The expanded commit description also seems to confuse this revert with the one in d84a1cecd
Not explained why we need to limit the parallel operations of the cketh minter canister, but I believe to have some sort of “rate limiting”.
I wasn’t able to find the piece code that does what’s stated in the 3.4 bullet point: The frequency of the timer will double (3min instead of 6min) as long as there is still work to do.
After giving it more thought, I believe the block proposal typo in 1f1f92959 is actually intentional, the subsequent assert! statement is only satisfied if the path doesn’t exist, which it shouldn’t due to the incorrect argument. However, it seems that the error messages have been mixed up.
Just to bump this out of curiosity since it was already executed it looks like nothing critical since the example they gave from testing seems to work as intended, I would guess it’s more of a refactoring typo since the assert!() macro uses the !proposal_path.exists(), and would fail, indicating that the backup of the non-final proposal failed in case the file exists.
So yeah feedback much appreciated @sat . Thanks
Not explained why we need to limit the parallel operations of the cketh minter canister, but I believe to have some sort of “rate limiting”.
The number of operations involved for withdrawals is rate-limited to ensure that there is not a huge queue of transactions that will take a very long time to be processed. This is because each withdrawal involves a threshold ECDSA signature + 1 HTTP outcalls to send the transaction via JSON-RPC.
I wasn’t able to find the piece code that does what’s stated in the 3.4 bullet point: The frequency of the timer will double (3min instead of 6min) as long as there is still work to do.
This point refers to this line in particular, where the interval for retrial defined by PROCESS_ETH_RETRIEVE_TRANSACTIONS_RETRY_INTERVAL is 3 minutes (instead of 6 minutes for the “regular” interval PROCESS_ETH_RETRIEVE_TRANSACTIONS_INTERVAL).
Thanks for the careful review! Don’t hesitate to let me know if some things are still unclear related to this topic.
Hi folks, I would like to address the question related to the commits in the “Networking” category.
As @ilbert correctly commented, the MR messaging indeed was wrong for 2e3589427. Indeed the MR is reverting dbf1b20. I apologise for the mistake.
As with respect to why it is reverted twice. We had problems with release qualification. So we had to revert the offending MR both on the branch that we wanted to rollout out and on the branch that is used for releasing new features.
Hi Wenzel, thank you for highlighting the issues and well done on establishing a productive external review group!
Regarding the finding from @cyberowl: nice catch! There is no bug in the actual consensus code, but the test was passing for a wrong reason. We, the consensus team, fixed the test already: the fix will appear soon on the public github repository.
