hashgraph / hedera-services

Crypto, token, consensus, file, and smart contract services for the Hedera public ledger
Apache License 2.0
313 stars 138 forks source link

`VirtualNodeCache`: wrong check in `deleteInternal()`? #5217

Open swirlds-automation opened 1 year ago

swirlds-automation commented 1 year ago

Calling throwIfLeafImmutable() in deleteInternal() is inconsistent with the logic of the class and looks like a copy-paste error. Changing it to throwIfInternalsImmutable(), which is also used in putInternal(), causes 9 unit tests to fail, however.

swirlds-automation commented 1 year ago

I think this is intentional and I have to remember why. Even though the name looks wrong. It has to do with what phase we delete internals in. I believe internals are only deleted during handle transaction, which is when leafs are mutable. Internals are created during hashing when leafs are immutable. author:rbair23, createdAt:2022-02-17T18:27:15Z, updatedAt=2022-02-17T18:27:15Z

swirlds-automation commented 1 year ago

deleteInternal() is called only from VirtualRootNode.remove(K key) so yes, it can happen only when leaves are mutable (and internals are immutable). That is confusing. Maybe the logic should be internalized in VirtualNodeCache without making deleteInternal() public with very specific conditions of when and how it could be invoked (making it an ad hoc method). author:OlegMazurov, createdAt:2022-02-17T18:49:02Z, updatedAt=2022-02-17T18:49:02Z

swirlds-automation commented 1 year ago

deleteInternal() is used in the same context and serves a similar purpose as clearLeafPath(long path). It should be renamed clearInternalPath and placed along with clearLeafPath. throwIfLeafImmutable() will not create confusion in that setting (helped by a clarifying comment). author:OlegMazurov, createdAt:2022-02-18T03:55:53Z, updatedAt=2022-02-18T03:55:53Z

swirlds-automation commented 1 year ago

migrated from: url=https://github.com/swirlds/swirlds-platform/issues/4825 author:OlegMazurov, #:4825, createdAt:2022-02-17T16:31:24Z, updatedAt=2023-02-22T03:15:05Z labels=Code Cleanup,Platform Virtual Map,Platform Data Structures,Migration:Data