Skip to content

On recovery, set UVM descriptor SVN to minimum of existing KV value and startup endorsements#7717

Merged
achamayou merged 15 commits intomainfrom
copilot/address-7716
Mar 6, 2026
Merged

On recovery, set UVM descriptor SVN to minimum of existing KV value and startup endorsements#7717
achamayou merged 15 commits intomainfrom
copilot/address-7716

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 6, 2026

During recovery, the node was unconditionally overwriting the UVM descriptor SVN with the value from the new node's endorsements. This could raise the SVN above what was previously stored, which is incorrect — recovery should never increase the SVN.

Changes

  • src/node/uvm_endorsements.h/.cpp: Extracted a parse_svn utility function that parses an SVN string to size_t, reused by both matches_uvm_roots_of_trust and the new recovery logic.
  • src/service/internal_tables_access.h: trust_node_uvm_endorsements now accepts a recovering flag. When true, reads the existing SVN from the KV for the same DID/feed and writes min(existing, new) using parse_svn. The feed entry is merged into the existing map for the DID, preserving other feeds rather than overwriting the entire map. A changed boolean tracks whether the map was actually modified (new feed, or SVN value differs), and the KV write is skipped entirely when nothing changed.
  • src/node/rpc/node_frontend.h: Pass recovering (already computed in the /create handler) to trust_node_uvm_endorsements.
  • CHANGELOG.md: Documented the behaviour change.

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 2 commits March 6, 2026 13:08
…ry (#7716)

Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
…dorsements

Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
Copilot AI changed the title [WIP] Address issue #7716 On recovery, set UVM descriptor SVN to minimum of existing KV value and startup endorsements Mar 6, 2026
@achamayou
Copy link
Copy Markdown
Member

@copilot I am sure we have SVN comparison logic already somewhere, please find that and use it, factor it out if necessary in its own function, but don't replicate it inline like that.

achamayou and others added 2 commits March 6, 2026 14:02
…f_trust and trust_node_uvm_endorsements

Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 6, 2026

@copilot I am sure we have SVN comparison logic already somewhere, please find that and use it, factor it out if necessary in its own function, but don't replicate it inline like that.

Extracted parse_svn(const std::string&) into src/node/uvm_endorsements.h/.cpp and refactored both matches_uvm_roots_of_trust and trust_node_uvm_endorsements to use it. Removed the inline <charconv>/from_chars duplication from internal_tables_access.h. See commit e7296cc.

Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
@achamayou achamayou marked this pull request as ready for review March 6, 2026 14:18
@achamayou achamayou requested a review from a team as a code owner March 6, 2026 14:18
Copilot AI review requested due to automatic review settings March 6, 2026 14:18
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR modifies the UVM endorsements trust logic during node recovery to use the minimum SVN (Security Version Number) between the existing stored value and the new node's startup endorsements. This prevents a recovery from inadvertently raising the SVN requirement.

Changes:

  • Extract parse_svn utility function into uvm_endorsements.h/.cpp, replacing duplicated inline parsing logic.
  • Add a recovering parameter to trust_node_uvm_endorsements that applies min(existing, new) SVN selection during recovery.
  • Pass the recovering flag from the /create handler and document the behavior change in CHANGELOG.md.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/node/uvm_endorsements.h Declares the new parse_svn utility function.
src/node/uvm_endorsements.cpp Implements parse_svn and refactors existing SVN parsing to use it.
src/service/internal_tables_access.h Adds recovering parameter and min-SVN logic to trust_node_uvm_endorsements.
src/node/rpc/node_frontend.h Passes recovering flag to trust_node_uvm_endorsements.
CHANGELOG.md Documents the new recovery SVN behavior.

You can also share your feedback on Copilot code review. Take the survey.

Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 6, 2026

@copilot I am sure we have SVN comparison logic already somewhere, please find that and use it, factor it out if necessary in its own function, but don't replicate it inline like that.

Already addressed in e7296ccparse_svn was extracted into src/node/uvm_endorsements.h/.cpp and both call sites use it.

@achamayou
Copy link
Copy Markdown
Member

@copilot capture a boolean value that tells us if we've actually updated the map (did or feed was not there, or svn has changed) and only write if we did make a change!

Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 6, 2026

@copilot capture a boolean value that tells us if we've actually updated the map (did or feed was not there, or svn has changed) and only write if we did make a change!

Done in b8b7f52. A changed boolean now tracks whether the feed is new or the SVN value differs; uvme->put is only called when changed is true. The recovery log message is also gated on changed.

@achamayou achamayou enabled auto-merge (squash) March 6, 2026 18:54
@achamayou achamayou merged commit 5a41a95 into main Mar 6, 2026
17 checks passed
@achamayou achamayou deleted the copilot/address-7716 branch March 6, 2026 19:20
Copilot AI added a commit that referenced this pull request Mar 9, 2026
…sting KV value and startup endorsements

This backport adapts the changes from PR #7717 (merged to main) for the release/6.x branch.

Changes:
- Extract parse_svn utility in uvm_endorsements.cpp/.h
- Refactor matches_uvm_roots_of_trust to use parse_svn
- trust_node_uvm_endorsements now accepts recovering flag; during recovery, SVN is set to min(existing, new)
- Pass recovering in node_frontend.h
- Add internal_tables_access_test unit test
- Update e2e_operations.py to bump SVN before recovery
- Add CHANGELOG entry under 6.0.24

Note: This branch contains 6.x-adapted code. Please retarget the PR base to release/6.x before merging.
Copilot AI added a commit that referenced this pull request Mar 9, 2026
…existing KV value and startup endorsements

Co-authored-by: maxtropets <16566519+maxtropets@users.noreply.github.com>
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.

5 participants