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

Make DataWord Great Again #796

Merged
merged 6 commits into from Apr 5, 2019

Conversation

Projects
None yet
6 participants
@ajlopezrsk
Copy link
Contributor

commented Mar 27, 2019

Internal data byte array is final.

Arithmetic operations returns a new DataWord, so the original data word is not changed.

Internal pool was removed

Many methods were remove, like clone()

@tinchou
Copy link
Contributor

left a comment

Could you fix the constructors as a whole? They have various issues, e.g it doesn't make sense to use a ByteBuffer in the one on line 68.

I would also ask that methods that aren't used in production (such as shifts) are not added to the codebase (YAGNI). If you also refactor the other methods (such as mul) to use them, then it's alright.

@tinchou
Copy link
Contributor

left a comment

There are still data leaks that don't make the DataWord class immutable.

E.g.:

  • The and method modifies this.data
  • getData doesn't return a copy
@tinchou

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

Please ensure at construction time that data has exactly 32 bytes.


private byte[] data; // Optimization, do not initialize until needed
private final byte[] data; // Optimization, do not initialize until needed

This comment has been minimized.

Copy link
@tinchou

tinchou Mar 27, 2019

Contributor

comment is outdated

This comment has been minimized.

Copy link
@ajlopezrsk

ajlopezrsk Mar 27, 2019

Author Contributor

Removed

program.disposeWord(outDataSize);
program.disposeWord(codeAddress);
program.disposeWord(gas);
if (config.isRskip103()) {

This comment has been minimized.

Copy link
@tinchou

tinchou Mar 27, 2019

Contributor

I some behavior here must be kept for consensus. Have you tried syncing mainnet and testnet with this code?

This comment has been minimized.

Copy link
@ajlopezrsk

ajlopezrsk Mar 27, 2019

Author Contributor

The removed disposeWord doesn't alter consensus.

Show resolved Hide resolved rskj-core/src/main/java/org/ethereum/vm/program/Program.java Outdated
Show resolved Hide resolved rskj-core/src/test/java/org/ethereum/vm/DataWordTest.java Outdated
@ajlopezrsk

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2019

Could you fix the constructors as a whole? They have various issues, e.g it doesn't make sense to use a ByteBuffer in the one on line 68.

I would also ask that methods that aren't used in production (such as shifts) are not added to the codebase (YAGNI). If you also refactor the other methods (such as mul) to use them, then it's alright.

Done

@tinchou tinchou force-pushed the redw branch from 5e2a4ad to 2ad7074 Apr 1, 2019

@tinchou

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

I just pushed a commit and approved this PR. Also rebased.

@tinchou tinchou dismissed their stale review via 94eb51f Apr 1, 2019

@tinchou tinchou force-pushed the redw branch from 2ad7074 to 94eb51f Apr 1, 2019

ajlopez and others added some commits Mar 25, 2019

DataWord refactor
DataWord new private constructor

Removing two newDataWord methods

Refactor DataWord or, xor

Refactor, redesign DataWord add, addmod

Refactor, redesign DataWord mul

Refactor, redesign DataWord div, sdiv

Refactor, redesign DataWord sub operation

Refactor, redesign DataWord exp, div, sdiv operations

Refactor, redesign DataWord signExtend

Refactor, redesign DataWord

Using DataWord.ZERO, DataWord.ONE

Additional DataWord tests; fixing DataWord.addmod

@tinchou tinchou force-pushed the redw branch from 94eb51f to 84992c0 Apr 5, 2019

@rskops

This comment has been minimized.

Copy link

commented Apr 5, 2019

SonarQube analysis reported 12 issues

  • MAJOR 4 major
  • MINOR 5 minor
  • INFO 3 info

Top 10 issues

  1. MAJOR GenesisLoader.java#L119: Refactor this code to not nest more than 3 if/for/while/switch/try statements. rule
  2. MAJOR VM.java#L1825: The Cyclomatic Complexity of this method "steps" is 11 which is greater than 10 authorized. rule
  3. MAJOR Program.java#L304: Method org.ethereum.vm.program.Program.memorySave(DataWord, DataWord) appears to call the same method on the same object redundantly rule
  4. MAJOR ProgramTrace.java#L81: Refactor this code to not nest more than 3 if/for/while/switch/try statements. rule
  5. MINOR SamplePrecompiledContract.java#L110: Define a constant instead of duplicating this literal "result" 3 times. rule
  6. MINOR TransactionExecutor.java#L325: Remove this use of "getContractDetails"; it is deprecated. rule
  7. MINOR TransactionExecutor.java#L469: Remove this use of "getContractDetails"; it is deprecated. rule
  8. MINOR ProgramTrace.java#L116: Return a copy of "structLogs". rule
  9. MINOR ProgramTrace.java#L120: Store a copy of "structLogs". rule
  10. INFO VM.java#L462: This method org.ethereum.vm.VM stores the value of a toString() call into a field rule
@tinchou

tinchou approved these changes Apr 5, 2019

Copy link
Contributor

left a comment

Re-approve after rebase

@diega diega merged commit ba327eb into master Apr 5, 2019

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
default Build finished.
Details
sonarqube SonarQube reported 12 issues, no criticals or blockers

@diega diega deleted the redw branch Apr 5, 2019

@aeidelman aeidelman added this to the Orchid v0.6.2 milestone Apr 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.