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.

Sign up for GitHub

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

feat: retry query signature verification in case cache is stale #801

Merged
merged 6 commits into from
Nov 17, 2023
+26 −5
Merged

feat: retry query signature verification in case cache is stale #801

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d55e6a3
feat: retry query signature verification in case cache is stale
krpeacock Nov 16, 2023
10297dd
todo test
krpeacock Nov 16, 2023
08b3c48
5 minute expiration, canisterId as key
krpeacock Nov 16, 2023
af22411
fetchSubnetKeys
krpeacock Nov 16, 2023
860298a
foreach async wasn't correctly awaited
krpeacock Nov 17, 2023
ce7e135
cleaning up
krpeacock Nov 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/generated/changelog.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ <h1>Agent-JS Changelog</h1>

<section>
<h2>Version x.x.x</h2>
<ul>
<li>feat: retry query signature verification in case cache is stale</li>
</ul>
<h2>Version 0.20.0</h2>
<ul>
<li>feat: uses expirable map for subnet keys in agent-js, with a timeout of 1 hour</li>
<li>chore: cleanup for node 20 development in agent-js</li>
Expand Down
2 changes: 2 additions & 0 deletions packages/agent/src/agent/http/http.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -790,3 +790,5 @@ test('retry requests that fail due to a network failure', async () => {
expect(mockFetch.mock.calls.length).toBe(4);
}
});

test.todo('retry query signature validation after refreshing the subnet node keys');
17 changes: 15 additions & 2 deletions packages/agent/src/agent/http/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,7 +532,20 @@ export class HttpAgent implements Agent {
if (!this.#verifyQuerySignatures) {
return query;
}
return this.#verifyQueryResponse(query, subnetStatus);

try {
return this.#verifyQueryResponse(query, subnetStatus);
} catch (error) {
// In case the node signatures have changed, refresh the subnet keys and try again
console.warn('Query response verification failed. Retrying with fresh subnet keys.');
const nodeId = Principal.from(query.signatures?.[0].identity);
this.#subnetKeys.delete(nodeId.toText());
krpeacock marked this conversation as resolved.
Show resolved Hide resolved
await this.fetchSubnetKeys(nodeId);
krpeacock marked this conversation as resolved.
Show resolved Hide resolved

const updatedSubnetStatus = this.#subnetKeys.get(nodeId.toText());
krpeacock marked this conversation as resolved.
Show resolved Hide resolved
return this.#verifyQueryResponse(query, updatedSubnetStatus);
Copy link
Member

Choose a reason for hiding this comment

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

Is it okay that there is no resolve(subnetStatus); call here, in between get and verifyQueryResponse, as in the above code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we are not within a Promise at this stage in the code (apart from the async query function itself). It's a little semantically incongruous if you're comparing this section to the above sections, however

}

}

/**
Expand All @@ -557,7 +570,7 @@ export class HttpAgent implements Agent {
const { status, signatures, requestId } = queryResponse;

const domainSeparator = new TextEncoder().encode('\x0Bic-response');
signatures?.forEach(sig => {
signatures?.forEach(async sig => {
const { timestamp, identity } = sig;
const nodeId = Principal.fromUint8Array(identity).toText();
let hash: ArrayBuffer;
Expand Down
Loading