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

feat: client api for pkarr record republishing #79

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

SHAcollision
Copy link
Contributor

@SHAcollision SHAcollision commented Feb 23, 2025

Fixes #78 .This PR implements the functionality needed to help clients ensure users' pkarr records have higher availability directly from the DHT. This PR does not add logic for the homeserver to republish its own key. This work should be done next.

  • Unchanged. On Signup:
    The signup() method call publishes a new pkarr record immediately. Nothing new here. This has always been the behaviour and also the only time ever a user pkarr record is published.

  • NEW on Signin:
    After signing in (i.e. after a token is successfully recovered), the client spawns a background task that republishes an existing record. It resolves the existing record once, uses the helper extract_host_from_record(...) to determine the current homeserver host, and then calls publish_homeserver() with the conditional IfOlderThan than will republish a record if it's missing or older than 4 days.

  • New Public Method to republish homeserver:
    The public method republish_homeserver(keypair, host) is now available for key management apps that know what the target homeserver should be (even if signin fails) and want to force a republishing after detecting the pkarr record is no longer available (e.g. signin failed due to unresolvable homeserver). This method allows republishing without having to trigger a full signup with the homeserver once again. It is also conservative in the use of the Dht as it will NOT publish the record if a recent (<4 days) record is found.

@SHAcollision SHAcollision self-assigned this Feb 23, 2025
Copy link
Collaborator

@SeverinAlexB SeverinAlexB left a comment

Choose a reason for hiding this comment

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

I like your use of comments. It's refreshing compared to Ars code.

Otherwise, I made comments but I am not fully read into all the code yet so I don't have perfect confidence yet that this is correct so I just trust you.

Comment on lines 121 to 123
let max_record_age_micros = self
.max_record_age_micros
.unwrap_or(4 * 24 * 60 * 60 * 1_000_000);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I strongly suggest do set the default republishing interval to 1hr for the time same sake. We can still increase it in the future after research but we should aim for safe at the moment.

Suggested change
let max_record_age_micros = self
.max_record_age_micros
.unwrap_or(4 * 24 * 60 * 60 * 1_000_000);
let max_record_age_micros = self
.max_record_age_micros
.unwrap_or(1 * 60 * 60 * 1_000_000);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. 4 days is extremely optimistic. They seem to last MUCH less.

Comment on lines 37 to 39
/// Maximum age in microseconds before a user record should be republished.
/// Defaults to 4 days.
max_record_age_micros: Option<u64>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why in microseconds? Sounds very excessive. What about just seconds?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, it's the standard in ntimestamp it seems so ignore me.

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, this is the standard for all timming we are using. It all comes from pubky-timestamp crate https://github.com/pubky/timestamp

@@ -85,10 +90,24 @@ impl Client {
}

/// Signin to a homeserver.
/// After a successful signin, a background task is spawned to republish the user's
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know so I ask: Can you even sign in without an active pkarr record pointing to the homeserver?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly, you can't. You can only sign in if you can resolve the pkarr record pointing to your homeserver.

However, you most likely will get it from a pkarr relay and after signing (activity), the client pushes it to the DHT again.

@SHAcollision
Copy link
Contributor Author

Thanks a lot for the review @SeverinAlexB . Let's leave this PR open as WIP a few more days until we run more research on eviction and spamming and gain more confidence on the solutions ( aka republisher service and client triggered republishing).

@SHAcollision
Copy link
Contributor Author

Managed to fix the wasm compilation errors. Apparently crate::Client and super::super::Client are the same, but only sometimes. Took me easily 3 hours to figure :`)

Fully tested in wasm as well and to be released in #81 v0.4.2-rc1

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.

feat: republishing of user pkarr records
2 participants