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

Claim name normalization #102

Closed
wants to merge 1 commit into from

Conversation

lbrynaut
Copy link
Contributor

Add lowercase and unicode normalization to claim name entries in the claim trie

Related to #65

@@ -282,29 +290,31 @@ bool CClaimTrie::haveClaimInQueue(const std::string& name, const COutPoint& outP
{
return false;
}
const std::string normalized_name = lbryNormalize(name, CHECK_HEIGHT(nValidAtHeight));
Copy link
Member

Choose a reason for hiding this comment

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

it looks like all the vars here are camelCase. should normalized_name be normalizedName?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, they can be.

@lbry-bot lbry-bot assigned lbrynaut and unassigned lbrynaut Mar 5, 2018
@lbrynaut lbrynaut force-pushed the normalized-name-fork branch 26 times, most recently from 08c57a7 to 22c96da Compare March 12, 2018 19:09
@lbrynaut lbrynaut force-pushed the normalized-name-fork branch 9 times, most recently from 0c68ab9 to 8a25f6a Compare March 14, 2018 16:52
@lbrynaut lbrynaut removed their assignment Mar 14, 2018
@@ -23,6 +28,9 @@
#define SUPPORT_QUEUE_NAME_ROW 'p'
#define SUPPORT_EXP_QUEUE_ROW 'x'

// hard fork height for claim name normalization
static const int LBRY_NORMALIZED_NAME_FORK_HEIGHT = 329320;

Choose a reason for hiding this comment

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

We should make this adjustable via chainparams.cpp so that we can define different block height for testing in regtest/testnet

@lyoshenka lyoshenka changed the title WIP: claim name normalization Claim name normalization Mar 27, 2018
@tzarebczan
Copy link
Contributor

@lbrynaut what are the next steps here?

@lbrynaut
Copy link
Contributor Author

@tzarebczan 1) The required ICU dependency build issue needs to be resolved (last I heard, not by me), 2) Unit tests need to be added, 3) Feedback needs to be applied.

I'd say it's stalled at the moment due to 1). I will resolve it eventually it if no one else does after some other work is complete. If someone else resolves it, let me know and I'll move on to 2).

@kauffj kauffj mentioned this pull request Mar 28, 2018
@kauffj
Copy link
Member

kauffj commented Mar 28, 2018

Apologies, I brought this up with the team but was unclear in who was responsible.

@IGassmann is going to look at #111 and see if he can crack this build issue.

If not, he will escalate to Grin or Kay.

@kauffj
Copy link
Member

kauffj commented Mar 28, 2018

@lbrynaut can 2 and 3 progress without 1?

@lbrynaut
Copy link
Contributor Author

@kauffj yes

@tzarebczan
Copy link
Contributor

@lbrynaut - to be clear, users will still be allowed to create claims with capital letters but this will ensure that the vanity/permanent URL resolution does not take into account case sensitivity?

@lbrynaut
Copy link
Contributor Author

lbrynaut commented Apr 2, 2018

@tzarebczan Users can create whatever they want, but internally, claim names are stored/referenced in a normalized way. So you could specify Test, tEst, TEST, tesT, which would all refer to the same claim after the fork. When I have time, I'll clean this up and add some unit tests to demonstrate.

@IGassmann
Copy link

I, unfortunately, wasn't able to resolve the build issue by now. Either @lyoshenka or @kaykurokawa should be able to resolve it more effectively. If any help is needed for the Travis configuration, I'm available.

@lbrynaut
Copy link
Contributor Author

lbrynaut commented Apr 4, 2018

Heads up -- I've been revisiting this and I think I'm going to abandon this work in favor of a new proposal. This approach has a lot of boundary conditions that are difficult to narrow down, cannot perform as well as I'd like, and may be overly complicated (in retrospect).

@lyoshenka
Copy link
Member

can we close this in favor of #159 ?

@lbrynaut
Copy link
Contributor Author

can we close this in favor of #159 ?

Yes.

@lbrynaut lbrynaut closed this Jun 13, 2018
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.

None yet

6 participants