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

contracts: update root cache upon renewal #389

Closed
wants to merge 1 commit into from

Conversation

peterjan
Copy link
Member

@peterjan peterjan commented May 8, 2024

This PR updates the root cache in the ContractManager when a contract gets renewed, essentially copying over the roots from existing to renewal. I'm keeping this as a DRAFT @n8maninger because it's merely to illustrate the problem, the real fix is (likely) something else entirely and this will also need a thorough regression test. I don't mind taking a stab at it but I felt this is probably something that you will want to write the fix for yourself.

Context:
In renterd we have a test called TestUploadDownloadSpending that fails locally with AppendSector: proof verification failed on the branch pj/subscription-api. The reason it fails is because hostd builds the proof of a recently renewed contract without sector roots. I sprinkled some logging in the host's contract manager and also the proof code in core and found that we seem to update the database just fine on renewal but the next time the contract updater fetches the roots from the store it returns 0 roots. Causing the host to try and build a proof where secRoots is empty causing it to fail when verifying. I considered the possibility that something was removing those roots but I added panics in all delete and trim methods and those aren't hit during the test.

DEBUG PJ: host: contract manager: store renew contract fcid:d0ddaa094b8082ec760af191a241403846b89136ee1b6343f42bda1044a4e434 -> fcid:9fe0027ea0f4e28861fffcc86a06a93969e49acf98fa18d60de7919f89293c7e
DEBUG PJ: store: renew fcid:d0ddaa094b8082ec760af191a241403846b89136ee1b6343f42bda1044a4e434 -> fcid:9fe0027ea0f4e28861fffcc86a06a93969e49acf98fa18d60de7919f89293c7e updated 3 roots
DEBUG PJ: host: contract updater:  fcid:9fe0027ea0f4e28861fffcc86a06a93969e49acf98fa18d60de7919f89293c7e append sector, new sector roots cnt 1
DEBUG PJ: host: contract updater: fcid:9fe0027ea0f4e28861fffcc86a06a93969e49acf98fa18d60de7919f89293c7e current sector roots cnt 1
DEBUG PJ: core0.2.3: BuildDiffProof
actions: [{Type:Append A:0 B:0 Data:[]}]
secRoots: []
treeHas: []
leafHas: []
DEBUG PJ: core0.2.3: VerifyDiffProof
actions: [{Type:Append A:0 B:0 Data:[]}]
numleav: 3
treeHas: []
leafHas: []
oldRoot: h:5927721564a242691a0ca9d58f810087ca838fe5205989417296cc6181939067
newRoot: h:c076a802cc2791cb30a710a1977e88e540abff86d091014117ed8e0321c43a66
appRoot: [h:c076a802cc2791cb30a710a1977e88e540abff86d091014117ed8e0321c43a66]
OK: false

Couple of things I find especially odd/interesting is that it passes on CI, so somehow this is a race condition, and also if we really consider the contract to be empty after renewal, even for a short period of time, this would wreak some serious havoc on production so it's probably somehow not that big of an issue or a really small window or something.

@peterjan peterjan requested a review from n8maninger May 8, 2024 12:01
@n8maninger
Copy link
Member

This seems to be an issue with the test, not with hostd

@n8maninger n8maninger closed this May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants