-
Notifications
You must be signed in to change notification settings - Fork 143
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
Make BlockChain<T>.GetStates() returns early for nonexistent addresses #197
Make BlockChain<T>.GetStates() returns early for nonexistent addresses #197
Conversation
Note: Although I added an ad-hoc field separated from Block type, this leads the order of storing multiple blocks to be forced unless there are information about corresponding addresses masks for these blocks to put in. (A mask for a block needs to be made on top of the mask for its previous block.) For example, if we want to retrieve a large amount of blocks out of order, we need to serialize the order of block insertions, from the genesis block to the topmost block, or retrieve pairs of block and its mask. Even if we retrieve blocks along with their masks, as masks are neither signed nor hashed with nonce, we implement extra validations for masks. It doesn't make only dealing with blocks complex and painful, but also these jobs bug-prone. So I suggest to put the mask field into the Block type, so that it is fully integrated into blockchain and all things are validated along with, in a consistent way.
Codecov Report
@@ Coverage Diff @@
## master #197 +/- ##
==========================================
- Coverage 87.42% 84.44% -2.98%
==========================================
Files 72 72
Lines 3292 3350 +58
==========================================
- Hits 2878 2829 -49
- Misses 414 521 +107
|
Libplanet/Blockchain/BlockChain.cs
Outdated
: Enumerable.Repeat((byte)0xff, Address.Size).ToArray() | ||
); | ||
|
||
Func<BitArray, BitArray, bool> isPossibilyIn = (addr, maskPat) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Func<BitArray, BitArray, bool> isPossibilyIn = (addr, maskPat) => | |
Func<BitArray, BitArray, bool> isPossiblyIn = (addr, maskPat) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed.
Libplanet/Blockchain/BlockChain.cs
Outdated
if (requestedAddresses.SetEquals(states.Keys)) | ||
// A set of addresses that are definitely not existent. | ||
ImmutableHashSet<Address> nonexistents = requestedAddresses | ||
.Where(pair => !isPossibilyIn(pair.Value, mask)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.Where(pair => !isPossibilyIn(pair.Value, mask)) | |
.Where(pair => !isPossiblyIn(pair.Value, mask)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed.
Libplanet/Blockchain/BlockChain.cs
Outdated
offset is HashDigest<SHA256> hash && | ||
Store.GetAddressesMask(hash) is Address a | ||
? a.ToByteArray() | ||
: Enumerable.Repeat((byte)0xff, Address.Size).ToArray() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this code can be precomputed, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh no, it definitely is a bug. I'm going to address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed this and amended the commit.
Libplanet/Blockchain/BlockChain.cs
Outdated
: Enumerable.Repeat((byte)0xff, Address.Size).ToArray() | ||
); | ||
|
||
Func<BitArray, BitArray, bool> isPossibilyIn = (addr, maskPat) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can use local function.
Func<BitArray, BitArray, bool> isPossibilyIn = (addr, maskPat) => | |
bool IsPossiblyIn(BitArray addr, BitArray maskPat) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed.
@@ -49,21 +51,57 @@ public override ICollection<Block<T>> Values | |||
throw new KeyNotFoundException(); | |||
} | |||
|
|||
Trace.Assert(block.Hash == key); | |||
Trace.Assert(block.Hash.Equals(key)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we prefer Equals()
instead of ==
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rider warned me that HashDigest<SHA256>
hadn't implemented this operator, so I made this change. AFAIK this operator is implemented through Uno.CodeGen though.
f71d987
to
f928bfd
Compare
Libplanet/Store/BlockSet.cs
Outdated
// (the store made in the older versions of Libplanet | ||
// may lack mask files), make the mask to match to | ||
// all possible addresses. | ||
prevMask = Enumerable.Repeat((byte)0xff, Address.Size) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also use WildcardMask here, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WildcardMask
is defined in the BlockChain<T>
class and it's private
. Making BlockSet<T>
depends on BlockChain<T>
also seems unnatural. So I'm going to define the same constant here too.
It's like we should define a distinct type like AddressMask
in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, as BlockChain<T>
already has depended on BlockSet<T>
(though the inverse is not true), I'm going to move the WildcardMask
constant to BlockSet<T>
and make it internal
instead of private
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed and amended the commit: 2317044.
f928bfd
to
2317044
Compare
@earlbread @longfin Could you review this again? |
…ntiation Avoid needless PublicKey instantiation (which is time expensive)
…netarium#197) * INTERNAL: update configmap-versions.yaml * INTERNAL: update kustomization.yaml
This patch completely fixes the bug #189. Besides the patch #192, this deals with nonexistent addresses.
This involves a mask-based index to approximates a set of
Addresses
. The key idea is that if a certain address had been ever used and included into the mask, the mask ensures to have only non-zero bits (i.e., trues) for the same positions that the address has non-zero bits. You can also think of it as a kind of bloom filter but no hash functions. The point is that although a mask cannot ensure if an address is in it but can ensure an address is definitely not in it.To store a mask for each block, I added a new parameter
Address addressesMask
toIStore.PutBlock<T>()
method, and a new method namedIStore.GetAddressesMask()
to query this. I wrote XML comments on these methods. Please read comments for API details.Although I added a mask as an ad-hoc field of
IStore
at this stage, I suggest put this information intoBlock<T>
type for the future so that mask is also validated within block's cryptographic hash. (More detailed proposal is in the commit message of e01da78.)