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.

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

feat(*): optimize old rewards withdrawal #5742

Merged

Conversation

halibobo1205
Copy link
Contributor

@halibobo1205 halibobo1205 commented Mar 4, 2024

What does this PR do?

  1. remove useNewRewardAlgorithmFromStart
  2. code clear
  3. exit when the calculation task gets an exception
  4. tools: add db root to calculate the data root
  5. make reward-vi root configurable

Why are these changes required?

see #5406 (comment).

This PR has been tested by:

  • Unit Tests
  • Manual Testing

Follow up

Extra details

logger.warn("Merkle root mismatch, expect: {}, actual: {}."
+ " If you are quite sure that there is no miscalculation (not on the main network)"
+ ", please configure 'storage.merkleRoot.reward-vi = {}'"
+ "(for a specific network such as Nile, etc.) to config.conf to fix the hints",
Copy link
Contributor

Choose a reason for hiding this comment

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

a small spelling error.'to config.conf' should be 'in config.conf'.

private Sha256Hash rewardViRoot = Sha256Hash.wrap(
ByteString.fromHex("9debcb9924055500aaae98cdee10501c5c39d4daa75800a996f4bdda73dbccd8"));
private static final String MAIN_NET_ROOT_HEX =
"9debcb9924055500aaae98cdee10501c5c39d4daa75800a996f4bdda73dbccd8";
Copy link
Contributor

Choose a reason for hiding this comment

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

this root hex is already configured in conf. here it is declared again. Should it be possible for this value declared in one place instead of two different files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to ensure that the main-net node does not need to change the configuration file.

checkValid();
return dbIterator.peekNext().getKey();
}

@Override
public byte[] getValue() {
checkState();
Copy link
Contributor

@317787106 317787106 Mar 4, 2024

Choose a reason for hiding this comment

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

There is a varible method close and a method with the same name in this class, maybe you should use different names to distinguish them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No changes for now.

}
} catch (Exception e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In which case shall it throw an Exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Such as too many open files, not enough disk space, etc.

private List<Leaf> leaves;
private Leaf root;

public static MerkleTree getInstance() {
Copy link
Contributor

@eodiandie eodiandie Mar 4, 2024

Choose a reason for hiding this comment

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

here it uses Double-Checked Locking to ensure that only one instance is created in a multi-threaded environment. However, this code suffers from concurrency issues. Double-Checked Locking is not always reliable in Java because it depends on the memory model and compiler optimizations.
A safer implementation of the singleton pattern uses static inner classes in a way that is thread-safe and efficient。
private static class MerkleTreeHolder { private static final MerkleTree instance = new MerkleTree(); } public static MerkleTree getInstance() { return MerkleTreeHolder.instance; }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This class has member variables and can not be used for concurrency, need to modify the usage scenarios while labeling them as not being able to be concurrent.

@halibobo1205 halibobo1205 force-pushed the feat/old_reward_cal_opt branch 3 times, most recently from 8b61106 to 03fa9d9 Compare March 4, 2024 12:50
@halibobo1205 halibobo1205 marked this pull request as ready for review March 5, 2024 03:51
@halibobo1205 halibobo1205 added this to the GreatVoyage-v4.7.4 milestone Mar 5, 2024
@halibobo1205 halibobo1205 merged commit fa46c4f into tronprotocol:release_v4.7.4 Mar 6, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants