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

fix: read only nodes along the path on non-existent key deletion #11071

Merged
merged 12 commits into from
Apr 16, 2024

Conversation

Longarithm
Copy link
Member

@Longarithm Longarithm commented Apr 15, 2024

Yet another cornercase fix: if deleted key doesn't exist, read only nodes required to prove its non-existence, thus only nodes along the path.
The chaos it caused is described on #11065 and discussed on Zulip.
This is not a protocol change, because currently nodes read on Trie::update don't impact costs, so we can modify current disk trie logic to follow the principle that only necessary nodes should be read.

Unfortunately disk tries logic, on descending, always subtracts child' memory usage from parent' memory usage and requires "backwards iteration" along the path to recompute potentially changed memory usages. I want to minimise changes, so I just introduce key_deleted to compute memory usages but avoid reads caused by node restructuring if key wasn't found. If I just return early, I get attempt to subtract with overflow on calc_memory_usage_and_store.

Memtrie logic is already correct because it computes memory usages after processing all keys, which is cleaner.

I also restructure trie consistency changes. Now these tests:

  • always use memtries by default, as this is stateless validation flow we want to test;
  • always include comparison of partial storages generated by disk tries, memtries and recorded storages;
  • use random probabilities and any insert/delete operations for existing/missing keys. Unfortunately with previous impl all test still can pass, but in like 9/10 runs at least some of them fails. These tests are random, however, so I don't target zero flakiness.

Also tests with/without flat storage are better parametrised.

mooori pushed a commit to mooori/nearcore that referenced this pull request Apr 16, 2024
<p>This PR was automatically created by Snyk using the credentials of a
real user.</p><br /><h3>Snyk has created this PR to upgrade react-router
from 6.20.1 to 6.21.0.</h3>

:information_source: Keep your dependencies up-to-date. This makes it
easier to fix existing vulnerabilities and to more quickly identify and
fix newly disclosed vulnerabilities when they affect your project.
<hr/>

- The recommended version is **5 versions** ahead of your current
version.
- The recommended version was released **21 days ago**, on 2023-12-13.


<details>
<summary><b>Release notes</b></summary>
<br/>
  <details>
    <summary>Package name: <b>react-router</b></summary>
    <ul>
      <li>
<b>6.21.0</b> - <a
href="https://snyk.io/redirect/github/remix-run/react-router/releases/tag/react-router-native%406.21.0">2023-12-13</a></br><p>[email protected]</p>
      </li>
      <li>
<b>6.21.0-pre.3</b> - <a
href="https://snyk.io/redirect/github/remix-run/react-router/releases/tag/react-router-native%406.21.0-pre.3">2023-12-06</a></br><p>[email protected]</p>
      </li>
      <li>
        <b>6.21.0-pre.2</b> - 2023-12-05
      </li>
      <li>
        <b>6.21.0-pre.1</b> - 2023-12-05
      </li>
      <li>
        <b>6.21.0-pre.0</b> - 2023-12-05
      </li>
      <li>
        <b>6.20.1</b> - 2023-12-01
      </li>
    </ul>
from <a
href="https://snyk.io/redirect/github/remix-run/react-router/releases">react-router
GitHub release notes</a>
  </details>
</details>


<details>
  <summary><b>Commit messages</b></summary>
  </br>
  <details>
    <summary>Package name: <b>react-router</b></summary>
    <ul>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/69ba50e06633fd4add234fb47f2d49b0a5ee41f9">69ba50e</a>
chore: Update version for release (near#11114)</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/ffd4373737a5bda3ad0fcebfa4895fbd8038a504">ffd4373</a>
Exit prerelease mode</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/ea0ffeef8a0c353f8ef402b5df2ac7c60a4d0b49">ea0ffee</a>
chore: Update version for release (pre) (near#11095)</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/fe3c071037b18f447dabb44fb24e4b1bce2a2a15">fe3c071</a>
Slight refactor to partial hydration to leverage state.initialized
properly (near#11094)</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/ed40ee9e61f3c4608bcc2e4cb1275c4b5dbd144e">ed40ee9</a>
Update release notes comparison link</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/1447eb03a740b765ce79079ebd832afb5d1b02d8">1447eb0</a>
Update release notes</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/373b30cdfb4a770e77f6ded25e94ba72fbde4e23">373b30c</a>
chore: Update version for release (pre) (near#11091)</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/f9d7ed62904766013d05a1642232d73e47f3bc27">f9d7ed6</a>
Fix server future plumbing</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/56b2944ef18307fa6d5e432b52eead38eeda3402">56b2944</a>
chore: Update version for release (pre) (near#11090)</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/558d7936cc8fe643374a2c9a9fdcf022e8c4c939">558d793</a>
Fix plumbing of future prop</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/1c6f3b31fcd49a9065298769d097df420216238e">1c6f3b3</a>
fix typos</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/ee5fcd54af4784d58191d97a7117fa4d75378946">ee5fcd5</a>
Generate release notes</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/ddc2b94c26ac4a12b5a8e37dc4179e1b10644e1b">ddc2b94</a>
chore: Update version for release (pre) (near#11089)</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/1752d84eb44f9e1218798a1e431be9b02bbef9ab">1752d84</a>
Enter prerelease mode</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/120de8c6c61af165cd09b3b785141336619aadf8">120de8c</a>
Merge branch &#x27;main&#x27; into release-next</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/149ad65a8dfb90b18835ec784792b6a86e4427a9">149ad65</a>
Add future.v7_relativeSplatPath flag (near#11087)</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/839df232462e710f5f3bdff1cac2dff9504820be">839df23</a>
Bump bundle</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/d04c54100c527dd84c1ca4b7059d38d2a4b4be99">d04c541</a>
Fix tests from conflicting PRs</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/0d2a38c7395d2434f9b8cf8100890b8ad6aad0b8">0d2a38c</a>
Support partial hydration for Remix clientLoader/clientAction
(near#11033)</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/cb53f41d4341fcc7ca053d89b5bda03a4fe95106">cb53f41</a>
Fix issue when rendering Link/NavLink outside of matched routes
(near#11062)</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/211c1ff4af0fbf18d79de71a59dd1b6fe410637e">211c1ff</a>
Bump bundle</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/e6814d5202beab18a259441144203727a3f13287">e6814d5</a>
Properly handle falsy error values in ErrorBoundary&#x27;s (near#11071)</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/dc7833c2beec923a26f2e54898f66fe81e524fa5">dc7833c</a>
Catch errors when trying to unwrap responses (near#11061)</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/7a5c0a7d91b6f62847418a46ec2d43e0e292a5b2">7a5c0a7</a>
Merge branch &#x27;release-next&#x27; into dev</li>
    </ul>

<a
href="https://snyk.io/redirect/github/remix-run/react-router/compare/8b1ee67ebc00fc3e778d5e36fe9bca140b576b5c...69ba50e06633fd4add234fb47f2d49b0a5ee41f9">Compare</a>
  </details>
</details>
<hr/>

**Note:** *You are seeing this because you or someone else with access
to this repository has authorized Snyk to open upgrade PRs.*

For more information: <img
src="https://api.segment.io/v1/pixel/track?data=eyJ3cml0ZUtleSI6InJyWmxZcEdHY2RyTHZsb0lYd0dUcVg4WkFRTnNCOUEwIiwiYW5vbnltb3VzSWQiOiJhNDI2MjMwYi1iOTMzLTQwZmYtYjU1YS1mM2U2YTM5MzE2ODciLCJldmVudCI6IlBSIHZpZXdlZCIsInByb3BlcnRpZXMiOnsicHJJZCI6ImE0MjYyMzBiLWI5MzMtNDBmZi1iNTVhLWYzZTZhMzkzMTY4NyJ9fQ=="
width="0" height="0"/>

🧐 [View latest project
report](https://app.snyk.io/org/near-ecosystem/project/e3526cdd-987a-4d84-a55d-30d275fc17bc?utm_source&#x3D;github&amp;utm_medium&#x3D;referral&amp;page&#x3D;upgrade-pr)

🛠 [Adjust upgrade PR
settings](https://app.snyk.io/org/near-ecosystem/project/e3526cdd-987a-4d84-a55d-30d275fc17bc/settings/integration?utm_source&#x3D;github&amp;utm_medium&#x3D;referral&amp;page&#x3D;upgrade-pr)

🔕 [Ignore this dependency or unsubscribe from future upgrade
PRs](https://app.snyk.io/org/near-ecosystem/project/e3526cdd-987a-4d84-a55d-30d275fc17bc/settings/integration?pkg&#x3D;react-router&amp;utm_source&#x3D;github&amp;utm_medium&#x3D;referral&amp;page&#x3D;upgrade-pr#auto-dep-upgrades)

<!---
(snyk:metadata:{"prId":"a426230b-b933-40ff-b55a-f3e6a3931687","prPublicId":"a426230b-b933-40ff-b55a-f3e6a3931687","dependencies":[{"name":"react-router","from":"6.20.1","to":"6.21.0"}],"packageManager":"npm","type":"auto","projectUrl":"https://app.snyk.io/org/near-ecosystem/project/e3526cdd-987a-4d84-a55d-30d275fc17bc?utm_source=github&utm_medium=referral&page=upgrade-pr","projectPublicId":"e3526cdd-987a-4d84-a55d-30d275fc17bc","env":"prod","prType":"upgrade","vulns":[],"issuesToFix":[],"upgrade":[],"upgradeInfo":{"versionsDiff":5,"publishedDate":"2023-12-13T21:34:17.308Z"},"templateVariants":[],"hasFixes":false,"isMajorUpgrade":false,"isBreakingChange":false,"priorityScoreList":[]})
--->

Co-authored-by: snyk-bot <[email protected]>
@Longarithm Longarithm changed the title draft: reproduce #11065 fix: read only nodes along the path on non-existent key deletion Apr 16, 2024
@Longarithm Longarithm marked this pull request as ready for review April 16, 2024 13:21
@Longarithm Longarithm requested a review from a team as a code owner April 16, 2024 13:21
@Longarithm Longarithm requested review from akhi3030, tayfunelmas, staffik and robin-near and removed request for akhi3030 April 16, 2024 13:21
@Longarithm Longarithm linked an issue Apr 16, 2024 that may be closed by this pull request
Copy link

codecov bot commented Apr 16, 2024

Codecov Report

Attention: Patch coverage is 93.77778% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 71.06%. Comparing base (4d506a7) to head (d3bedf8).
Report is 11 commits behind head on master.

Files Patch % Lines
core/store/src/trie/insert_delete.rs 82.60% 8 Missing and 4 partials ⚠️
core/store/src/trie/trie_recording.rs 98.34% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11071      +/-   ##
==========================================
- Coverage   71.14%   71.06%   -0.08%     
==========================================
  Files         761      763       +2     
  Lines      152857   153082     +225     
  Branches   152857   153082     +225     
==========================================
+ Hits       108748   108791      +43     
- Misses      39672    39851     +179     
- Partials     4437     4440       +3     
Flag Coverage Δ
backward-compatibility 0.24% <0.00%> (-0.01%) ⬇️
db-migration 0.24% <0.00%> (-0.01%) ⬇️
genesis-check 1.43% <0.00%> (-0.01%) ⬇️
integration-tests 36.82% <19.11%> (-0.02%) ⬇️
linux 69.49% <93.77%> (-0.13%) ⬇️
linux-nightly 70.55% <93.77%> (-0.09%) ⬇️
macos 54.16% <93.75%> (+1.52%) ⬆️
pytests 1.66% <0.00%> (-0.01%) ⬇️
sanity-checks 1.44% <0.00%> (-0.01%) ⬇️
unittests 66.74% <93.75%> (-0.06%) ⬇️
upgradability 0.29% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Longarithm Longarithm added this pull request to the merge queue Apr 16, 2024
Merged via the queue into near:master with commit 05b991c Apr 16, 2024
28 of 29 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.

MissingTrieValue error in mocknet running statelessnet protocol
3 participants