Project

General

Profile

Task #1640

ImplicitSha256DigestComponent

Added by Junxiao Shi almost 7 years ago. Updated over 6 years ago.

Status:
Closed
Priority:
Normal
Category:
Base
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:
12.00 h

Description

Support ImplicitSha256DigestComponent type.

This task includes:

  • develop new component class
  • extend Name class to allow new component type
  • extend URI encoder and decoder
  • extend PIT matching algorithm

Related issues

Blocked by NDN Specifications - Task #2011: ImplicitSha256DigestComponentClosedAlex Afanasyev

Actions
Blocks NFD - Task #2118: ContentStore matching test cases: update for ImplictSha256DigestComponentClosedJunxiao Shi

Actions
Blocks NFD - Task #1706: Reduce implicit digest computation in ContentStoreClosedJunxiao Shi

Actions
#1

Updated by Junxiao Shi over 6 years ago

  • Blocked by Task #2011: ImplicitSha256DigestComponent added
#2

Updated by Junxiao Shi over 6 years ago

  • Subject changed from NumberComponent and ImplicitSha256DigestComponent to ImplicitSha256DigestComponent
  • Description updated (diff)
#3

Updated by Junxiao Shi over 6 years ago

  • Assignee set to Alex Afanasyev
#4

Updated by Alex Afanasyev over 6 years ago

  • Status changed from New to In Progress
  • % Done changed from 0 to 100
#5

Updated by Alex Afanasyev over 6 years ago

  • Status changed from In Progress to Code review
#6

Updated by Junxiao Shi over 6 years ago

Change-Id: I756c4b94196cf031da98b5689bd60630533dfeb3 doesn't complete this Task.

In particular, the following is missing:

  • change Data::getFullName to append ImplicitSha256DigestComponent instead of NameComponent
  • extend PIT matching algorithm, with test cases
  • in Interest Name, permit ImplicitSha256DigestComponent only as the last component
  • in Data Name, forbid ImplicitSha256DigestComponent
  • in FinalBlockId, forbid ImplicitSha256DigestComponent

These should be added in future commit(s) also under this Task.

#7

Updated by Alex Afanasyev over 6 years ago

  • % Done changed from 100 to 40
#8

Updated by Alex Afanasyev over 6 years ago

  • Target version set to v0.3
#9

Updated by Junxiao Shi over 6 years ago

I have some design questions about I756c4b94196cf031da98b5689bd60630533dfeb3:

  • Why is implicit digest component a state of name::Component class, instead of a subclass?
  • Given that implicit digest is always SHA256, is it necessary for "Sha256" to appear in function names, or can we drop it to make function names shorter?
#10

Updated by Alex Afanasyev over 6 years ago

Intention of including "Sha256" is to allow (in the future) replace the hashing algorithm, when/if sha256 becomes insecure.

With this intention, "sha256" needs to be included somewhere, either in the name or in the parameters somewhere.

#11

Updated by Junxiao Shi over 6 years ago

This question from note-9 is unanswered:

Why is implicit digest component a state of name::Component class, instead of a subclass?

#12

Updated by Alex Afanasyev over 6 years ago

Why not? Everything is name component, just some name components have special type.

It could have been a subclass, though I don't see much need for that. The current way (digest as a state) aligns with naming convention based elements, even though they do not have special types assigned (yet?).

Also, having it as a subclass may create some efficiency problems: something in the class must become virtual and Name parsing would need to create special instances for some name components, or there would be a need for creating an instance of digest component class before accessing the value. Given the only purpose for the special type is to provide disambiguation and there are no really any special handling (except converting to/from URI), there is no need for special subclass.

#13

Updated by Junxiao Shi over 6 years ago

  • Blocks Task #2118: ContentStore matching test cases: update for ImplictSha256DigestComponent added
#14

Updated by Junxiao Shi over 6 years ago

The pending commit commit:6e6f8a394aa5916517b4ef2bed6e6cbcc193d52c, when rebased upon commit:1805826b47c44177b7f42484b4970b3e9c94cfe7, causes segfault in NFD test initialization.
The problem is in NFD/tests/daemon/mgmt/validation-common.hpp.
It's equivalent to the following snippet:

#include <ndn-cxx/name.hpp>

using ndn::Name;

class A
{
public:
  static const Name s_name;
};

const Name A::s_name("/A");

int
main()
{
  return 0;
}

When Name is constructed statically, the order of constructing this static Name and SHA256DIGEST_PREFIX in ndn-cxx/src/name-component.cpp is undetermined.

Therefore, SHA256DIGEST_PREFIX could be unavailable when it's needed by Component::fromEscapedString.

I've tested that changing SHA256DIGEST_PREFIX to be a private static member of Component class does not solve this problem.
The following solution works:

static const std::string&
getSha256DigestUriPrefix()
{
  static const std::string prefix = "sha256digest=";
  return prefix;
}

// and use getSha256DigestUriPrefix() instead of SHA256DIGEST_PREFIX
#15

Updated by Junxiao Shi over 6 years ago

When Data::getFullName is changed to append ImplicitSha256DigestComponent, ContentStore and InMemoryStorage test cases will be broken.
Please make those test cases as expected failures.
I'll fix them in #2118.

#16

Updated by Junxiao Shi over 6 years ago

  • Blocks Task #1706: Reduce implicit digest computation in ContentStore added
#17

Updated by Alex Afanasyev over 6 years ago

  • Assignee changed from Alex Afanasyev to Jiewen Tan
  • % Done changed from 40 to 80

Jiewen, can you fix the test cases in http://gerrit.named-data.net/#/c/1407?

#18

Updated by Junxiao Shi over 6 years ago

As I said in note-15, please don't fix any ContentStore and InMemoryStorage test cases in this Task. Just mark them as expected failures. I'll fix them in #2118.

#19

Updated by Alex Afanasyev over 6 years ago

  • Assignee changed from Jiewen Tan to Alex Afanasyev
  • % Done changed from 80 to 100

Missed that. Will change test assertions to WARN and add /// @todo statement

#20

Updated by Junxiao Shi over 6 years ago

Missed that. Will change test assertions to WARN and add /// @todo statement

Don't change assertions to WARN. Use expected failures.

#21

Updated by Alex Afanasyev over 6 years ago

I will add these to normal test cases, but this doesn't work with templated test cases :-(

#22

Updated by Alex Afanasyev over 6 years ago

What did you mean by PIT matching algorithm? Interest::matchesData? Or something inside NFD?

#23

Updated by Junxiao Shi over 6 years ago

I think Interest::matchesData shall be sufficient because client's PIT is using linked list.

#24

Updated by Junxiao Shi over 6 years ago

  • Status changed from Code review to Closed

Also available in: Atom PDF