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

Allow multiple delegates. #3301

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

arik-so
Copy link
Contributor

@arik-so arik-so commented Mar 17, 2024

This PR modifies server behavior to return the content of the first delegate it finds, absent which it returns the inscription's own content.

@arik-so arik-so marked this pull request as draft March 17, 2024 04:17
let inscription_override = inscription
.delegates()
.iter()
.find_map(|delegate| index.get_inscription_by_id(*delegate).unwrap_or(None));
Copy link
Contributor Author

@arik-so arik-so Mar 20, 2024

Choose a reason for hiding this comment

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

I'm really worried about this being a DoS vector. It would be fairly trivial to inscribe something with a couple thousand non-existent delegates. Any thoughts on whether it's a) a non-issue, b) makes the multi-delegate approach altogether unviable, or c) there are some mitigating strategies? @raphjaph

Copy link

@cryptocj cryptocj Mar 22, 2024

Choose a reason for hiding this comment

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

Can we add a limit number to avoid the Dos concern?
And extract a function to remove the repeat code below.

fn find_inscription<'a>(delegates: &'a [usize], index: &'a Index, max_length: usize) -> Option<&'a Inscription> {
    delegates
        .iter()
        .take(max_length)
        .find_map(|delegate| index.get_inscription_by_id(*delegate).unwrap_or(None))
}

// Usage
let inscription_override = find_inscription(inscription.delegates(), &index, max_length);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course, the question is just what the limit should be. That's why I think @casey or @raphjaph should weigh in.

@arik-so
Copy link
Contributor Author

arik-so commented Mar 20, 2024

This PR needs a test that, on the one hand, would allow the creation a custom inscription witness with multiple delegates, while on the other hand allowing to query the /content API endpoint.

The difficulty lies in the fact that Inscription::to_witness and InscriptionId::value are only available in ord::src, whereas TestServer is only available within ord::test. It seems like an inelegant merging of responsibilities to publicly expose the inscription-related methods, or to copy TestServer into ord::src, so if this is worth pursuing, I'm open to creative suggestions from the maintainers.

@arik-so arik-so marked this pull request as ready for review November 24, 2024 06:34
@casey
Copy link
Collaborator

casey commented Nov 26, 2024

I think this is fine, and I'm not to worried about the DoS potential. If it becomes a problem because someone created an inscription with a million delegates, we could just introduce a cap on the number of delegates that ordinals.com will respect. We could also be proactive, and limit it to, say, ten delegates, and just ignore any after that.

We've talked about recursive delegates before, see #2922. And I guess this probably works fine with that.

However, I'm not totally sure I understand the motivation for this feature. Can you open an issue to discuss it and tag me? I want to make sure that I understand what problem this solves or benefit it adds, and that whatever we do is the best possible solution.

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.

4 participants