neo icon indicating copy to clipboard operation
neo copied to clipboard

Fixed hashcode for StackItem

Open cschuchardt88 opened this issue 1 year ago • 8 comments

Description

Close https://github.com/neo-project/neo/issues/3543

I don't know if this was bug in the test if someone could answer that. See below. Either way the logic for doing byte array or spans was currently incorrect for HashCode.

itemA = new VM.Types.Buffer(1);
itemB = new VM.Types.Buffer(1);

Assert.IsTrue(itemA.GetHashCode() != itemB.GetHashCode()); // this line here

Type of change

  • [x] Optimization (the change is only an optimization)
  • [ ] Style (the change is only a code style for better maintenance or standard purpose)
  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [x] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [ ] This change requires a documentation update

How Has This Been Tested?

  • [x] Unit Tests

Checklist:

  • [x] My code follows the style guidelines of this project
  • [x] I have performed a self-review of my code
  • [x] I have commented my code, particularly in hard-to-understand areas
  • [x] I have made corresponding changes to the documentation
  • [x] My changes generate no new warnings
  • [x] I have added tests that prove my fix is effective or that my feature works
  • [x] New and existing unit tests pass locally with my changes
  • [x] Any dependent changes have been merged and published in downstream modules

cschuchardt88 avatar Oct 13 '24 08:10 cschuchardt88

Why don't return just the length?

lengths can be the same for different things. You want to treat everything your comparing to have a unique value for the same item. Hints the name HashCode

cschuchardt88 avatar Oct 13 '24 11:10 cschuchardt88

Why don't return just the length?

lengths can be the same for different things. You want to treat everything your comparing to have a unique value for the same item. Hints the name HashCode

But it's a valid hash, when the hash is the same, Equals is called.

shargon avatar Oct 13 '24 17:10 shargon

Why don't return just the length?

lengths can be the same for different things. You want to treat everything your comparing to have a unique value for the same item. Hints the name HashCode

But it's a valid hash, when the hash is the same, Equals is called.

If you implement Equals(T), you should also override the base class implementations of Equals(Object) and GetHashCode() so that their behavior is consistent with that of the Equals(T) method. If you do override Equals(Object), your overridden implementation is also called in calls to the static Equals(System.Object, System.Object) method on your class. In addition, you should overload the op_Equality and op_Inequality operators. This ensures that all tests for equality return consistent results.

Reference : https://learn.microsoft.com/en-us/dotnet/api/system.iequatable-1.equals?view=net-8.0

Also GetHashCode is used in hashtables.

cschuchardt88 avatar Oct 13 '24 17:10 cschuchardt88

Description

I don't know if this was bug in the test if someone could answer that. See below. Either way the logic for doing byte array or spans was currently incorrect for HashCode.

itemA = new VM.Types.Buffer(1);
itemB = new VM.Types.Buffer(1);

Assert.IsTrue(itemA.GetHashCode() != itemB.GetHashCode()); // this line here

Type of change

  • [x] Optimization (the change is only an optimization)
  • [ ] Style (the change is only a code style for better maintenance or standard purpose)
  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [x] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • [ ] This change requires a documentation update

How Has This Been Tested?

  • [x] Unit Tests

Checklist:

  • [x] My code follows the style guidelines of this project
  • [x] I have performed a self-review of my code
  • [x] I have commented my code, particularly in hard-to-understand areas
  • [x] I have made corresponding changes to the documentation
  • [x] My changes generate no new warnings
  • [x] I have added tests that prove my fix is effective or that my feature works
  • [x] New and existing unit tests pass locally with my changes
  • [x] Any dependent changes have been merged and published in downstream modules

I don't see any wrong there, hashCode is a hash to reduce the calls to equal method, must be fast as possible, and should return the same value when the value could be the same.

shargon avatar Oct 13 '24 20:10 shargon

itemA.GetHashCode() != itemB.GetHashCode() is saying there not equal, even through they are. That's not a good thing? Or is that by design?

cschuchardt88 avatar Oct 13 '24 21:10 cschuchardt88

itemA.GetHashCode() != itemB.GetHashCode() is saying there not equal, even through they are. That's not a good thing? Or is that by design?

Yes, you are right, they should return the same value for speed up

shargon avatar Oct 14 '24 07:10 shargon

@shargon I added a simple hashing. Please check code again.

cschuchardt88 avatar Oct 15 '24 18:10 cschuchardt88

@shargon please wait, this pr can cause problem, please wait for a sec.

Jim8y avatar Oct 23 '24 11:10 Jim8y

@cschuchardt88

My primary concern here is that this change may affect EQUAL-alike VM opcodes behaviour. From the previous discussions I see that the behaviour should be the same, but I'd insist to check this patch on the existing mainnet/testnet data and ensure that this change does not affect N3 chains states.

I have run this bench on the devpack test contracts, it behaves the same. But you are correct, @superboyiii may you please check this pr on mainnet and testnet?

Jim8y avatar Oct 24 '24 12:10 Jim8y

My primary concern here is that this change may affect EQUAL-alike VM opcodes behaviour. From the previous discussions I see that the behaviour should be the same, but I'd insist to check this patch on the existing mainnet/testnet data and ensure that this change does not affect N3 chains states.

Equals works as before

shargon avatar Oct 24 '24 13:10 shargon

It's also compatible on testnet. LGTM

superboyiii avatar Nov 04 '24 06:11 superboyiii