Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WIP]Fast Finality: reward distribution and slash parts #8

Merged
merged 18 commits into from
Mar 16, 2022
Merged

[WIP]Fast Finality: reward distribution and slash parts #8

merged 18 commits into from
Mar 16, 2022

Conversation

pythonberg1997
Copy link

pythonberg1997
pythonberg1997 commented Mar 4, 2022
View reviewed changes
contracts/BSCValidatorSet.sol
@@ -1,5 +1,5 @@
pragma solidity 0.6.4;

pragma experimental ABIEncoderV2;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how much of an impact this will have

realuncle
realuncle reviewed Mar 7, 2022
View reviewed changes
contracts/SlashIndicator.sol Outdated

require(IBSCValidatorSet(VALIDATOR_CONTRACT_ADDR).isCurrentValidator(_evidence.valAddr), "not current validator");

(address[] memory vals, bytes[] memory voteAddrs) = IBSCValidatorSet(VALIDATOR_CONTRACT_ADDR).getMiningValidators();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

emm, we need to discuss this. getMiningValidators only fetch the validator info about this epoch round, we should punish the validator even it is not in this round.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting for solution

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a new funciont named getLivingValidators in BSCValidatorSet.sol

pythonberg1997
pythonberg1997 commented Mar 8, 2022
View reviewed changes
truffle-config.js Outdated
@@ -46,6 +46,7 @@ module.exports = {
host: "127.0.0.1", // Localhost (default: none)
port: 8545, // Standard Ethereum port (default: none)
network_id: "*", // Any network (default: none)
gas: 10000000, // Slash test will cost 6790000 as most
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need concern here

realuncle
realuncle reviewed Mar 10, 2022
View reviewed changes
contracts/BSCValidatorSet.sol Outdated

modifier oncePerBlock() {
require(block.number > previousHeight, "can not slash twice in one block");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"can not slash twice in one block" description is wrong.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

realuncle
realuncle reviewed Mar 10, 2022
View reviewed changes
contracts/BSCValidatorSet.sol Outdated
Validator[] validatorSet;
ValidatorExtra[] _validatorExtraSet;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just use Address[] to save the gas?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

realuncle
realuncle reviewed Mar 10, 2022
View reviewed changes
contracts/BSCValidatorSet.sol Outdated
/*********************** Cross Chain App Implement **************************/
function handleSynPackage(uint8, bytes calldata msgBytes) onlyInit onlyCrossChainContract initValidatorExtraSet external override returns(bytes memory responsePayload) {
function handleSynPackage(uint8, bytes calldata msgBytes) onlyInit onlyCrossChainContract external override returns (bytes memory responsePayload) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why delete initValidatorExtraSet?

Copy link
Author

@pythonberg1997 pythonberg1997 Mar 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

realuncle
realuncle reviewed Mar 10, 2022
View reviewed changes
contracts/BSCValidatorSet.sol Outdated
emit validatorJailed(v.consensusAddress);
return CODE_OK;
}

function updateValidatorSet(Validator[] memory validatorSet) internal returns (uint32) {
function updateValidatorSet(Validator[] memory validatorSet, ValidatorExtra[] memory _validatorExtraSet) internal returns (uint32) {
Copy link
Collaborator

@realuncle realuncle Mar 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change to addressp[] memory _validatorBlsKeys?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

realuncle
realuncle reviewed Mar 10, 2022
View reviewed changes
contracts/BSCValidatorSet.sol Outdated
emit batchTransfer(crossTotal);
} else {
emit batchTransferFailed(crossTotal, "batch transfer return false");
(Validator[] memory validatorSetTemp, ValidatorExtra[] memory validatorExtraSetTemp) = _forceMaintainingValidatorsExit(validatorSet, _validatorExtraSet);
Copy link
Collaborator

@realuncle realuncle Mar 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible only return address[] too?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

realuncle
realuncle reviewed Mar 10, 2022
View reviewed changes
contracts/BSCValidatorSet.sol Outdated
_numOfCabinets = INIT_NUM_OF_CABINETS;
}

address[] memory validators = getValidators();
bytes[] memory voteAddrs = new bytes[](validators.length);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we wrap a function for this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

realuncle
realuncle reviewed Mar 10, 2022
View reviewed changes
contracts/BSCValidatorSet.sol Outdated
@@ -567,81 +679,54 @@ contract BSCValidatorSet is IBSCValidatorSet, System, IParamSubscriber, IApplica
require(newNumOfCabinets > 0, "the numOfCabinets must be greater than 0");
require(newNumOfCabinets <= MAX_NUM_OF_VALIDATORS, "the numOfCabinets must be less than MAX_NUM_OF_VALIDATORS");
numOfCabinets = newNumOfCabinets;
} else if (Memory.compareStrings(key, "finalityRewardRatio")) {
require(value.length == 32, "length of n mismatch");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

length of n mismatch ==> length of finalityRewardRatio mismatch

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

gothery001
gothery001 reviewed Mar 10, 2022
View reviewed changes
contracts/BSCValidatorSet.sol Outdated
Comment on lines 161 to 168
(IbcValidatorSetPackage memory validatorSetPkg, bool valid) = decodeValidatorSetSynPackage(INIT_VALIDATORSET_BYTES);
require(valid, "failed to parse init validatorSet");
for (uint i = 0;i<validatorSetPkg.validatorSet.length;i++) {
ValidatorExtra memory _validatorExtra;
for (uint i = 0; i < validatorSetPkg.validatorSet.length; i++) {
_validatorExtra.voteAddress = validatorSetPkg.voteAddrs[i];
currentValidatorSet.push(validatorSetPkg.validatorSet[i]);
currentValidatorSetMap[validatorSetPkg.validatorSet[i].consensusAddress] = i+1;
validatorExtraSet.push(_validatorExtra);
currentValidatorSetMap[validatorSetPkg.validatorSet[i].consensusAddress] = i + 1;
}
expireTimeSecondGap = EXPIRE_TIME_SECOND_GAP;
finalityRewardRatio = FINALITY_REWARD_RATIO;
alreadyInit = true;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should not add any code in init(), this is a contract that will be upgraded on BSC, but init() only be executed while deployed. If we add some code on init(), it will cause tests out of action

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

gothery001
gothery001 reviewed Mar 11, 2022
View reviewed changes
contracts/BSCValidatorSet.sol Outdated
}

function getMiningValidators() public view returns(address[] memory) {
function getMiningValidators() external initValidatorExtraSet override returns (address[] memory, bytes[] memory) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is view func, we could not remove view

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

contracts/BSCValidatorSet.sol Outdated
}
}

function getLivingValidators() external initValidatorExtraSet override returns (address[] memory, bytes[] memory) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is view func, we could not remove view

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@realuncle realuncle changed the base branch from develop to fast_finality March 15, 2022 12:42
realuncle
realuncle reviewed Mar 15, 2022
View reviewed changes
contracts/BSCValidatorSet.sol
} else {
for (uint i = 0; i < n; i++) {
if (!currentValidatorSet[i].jailed) {
consensusAddrs[living] = currentValidatorSet[i].consensusAddress;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some duplicated code here, try reduce them.

@realuncle realuncle merged commit 55ea201 into Blockorus:fast_finality Mar 16, 2022
pythonberg1997 referenced this pull request in node-real/bsc-genesis-contract Apr 28, 2022
@pythonberg1997
[WIP]Fast Finality: reward distribution and slash parts (#8)
96107ce 
* [WIP]Fast Finality: reward distribution and slash parts

* Update codes

* Fix review comments and update test

* Fix code errors related contract SystemReward and BSCValidatorSet

* Fix code errors related invalid opcode and add new api

* Optimize gasused of updateValidator and update test

* Optimize gasused of updateValidator and update test

* Minor error fixed

* Fix review comments and update scripts

* Fix init issue

* Fix external view issue and update test

* Fix review comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gothery001 gothery001 gothery001 left review comments

@realuncle realuncle realuncle approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pythonberg1997 @gothery001 @realuncle