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

Proof of index part 2 #488

Merged
merged 14 commits into from
Oct 14, 2021
Merged

Proof of index part 2 #488

merged 14 commits into from
Oct 14, 2021

Conversation

jiqiang90
Copy link
Contributor

@jiqiang90 jiqiang90 commented Sep 21, 2021

Closes #410

TODO:

  • Fix comment issues
  • Folk MMR repo, fix relate bugs and test
  • Test if we set mmr to nullable will not cause conflict in our current projects
  • Solve when adding large number of blocks to filebaseDb, JavaScript heap out of memory issue

if (poiBlock.mmrRoot === null) {
poiBlock.mmrRoot = mmrValue;
await poiBlock.save();
} else if (u8aToHex(poiBlock.mmrRoot) !== u8aToHex(mmrValue)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to check Uint8Array value here, without u8aTohex this report two value is not the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

use u8aEq

get mmrPath(): string {
return this._config.mmrPath
? this._config.mmrPath
: path.join(__dirname, `../../../../mmrs/${this.subqueryName}.mmr`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The default path better is related to cwd not the js file.

@@ -48,7 +48,7 @@ export function PoiFactory(sequelize: Sequelize, schema: string): PoiRepo {
},
mmrRoot: {
type: DataTypes.BLOB,
allowNull: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this might cause compatibility issue for user already have POI table initialized

@@ -148,6 +146,7 @@ export class IndexerManager {
await this.ensureMetadata(this.subqueryState.dbSchema);
if (this.nodeConfig.proofOfIndex) {
await this.poiService.init(this.subqueryState.dbSchema);
await this.mmrService.init(this.subqueryState.dbSchema);
Copy link
Collaborator

Choose a reason for hiding this comment

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

can run them together


const logger = getLogger('mmr');
const DEFAULT_WORD_SIZE = 32;
const DEFAULT_LEAF = Buffer.from('0x000000000000000000000000000000');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const DEFAULT_LEAF = Buffer.from('0x000000000000000000000000000000');
const DEFAULT_LEAF = Buffer.alloc(32);

`Append Mmr failed, input data length should be ${DEFAULT_WORD_SIZE}`,
);
}
const blockLeafHeight = poiBlock.id - this.blockOffset - 1; //leaf index alway -1
Copy link
Collaborator

Choose a reason for hiding this comment

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

I found the blockOffset you implemented is '0', shouldn't it be '1' ? if we made this change, do we still need -1 here?

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 think offset to 0 is make more sense than 1.

poiBlock.id - this.blockOffset should give us the the total number of blocks in the mmr (aka, leafLength)

blockOffset also used in line 46,

    this.mmrStartHeight =
      (await this.fileBasedMmr.getLeafLength()) + this.blockOffset;

When mmr service initialization, we want to repeat calculate previous mmr value for validation. Eg, last fileDb mmr block height is 100, we want it repeat generate mmr for block 100 again and compare to value in poi table.
Therefore in this case, it start from 100 + 0 (which is equal to initial start block -1) .

And If we changed the blockOffset to 1 (which is equal to initial start block), it start from 100 + 1 (offset) -1 (to previous block).

if (poiBlocks.length !== 0) {
for (const block of poiBlocks) {
if (this.mmrStartHeight + 1 < block.id) {
const nextAppendHeight = this.mmrStartHeight + 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nextAppendHeight is only used once, can make it inline

private metadataRepo: MetadataRepo;
private fileBasedMmr: MMR;
private poiRepo: PoiRepo;
private mmrStartHeight: number;
Copy link
Collaborator

Choose a reason for hiding this comment

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

can u use a better name, because mmrStartHeight doesn't sound like it will be changed.

async syncFileBaseFromPoi(): Promise<void> {
while (!this.isShutdown) {
if (!this.poiRepo || !this.fileBasedMmr) {
await delay(10);
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it actually possible?

@@ -0,0 +1,42 @@
// Copyright 2020-2021 OnFinality Limited authors & contributors
Copy link
Collaborator

Choose a reason for hiding this comment

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

we shouldn't claim copyright for this file I believe.

@@ -0,0 +1,162 @@
// Copyright 2020-2021 OnFinality Limited authors & contributors
Copy link
Collaborator

Choose a reason for hiding this comment

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

we shouldn't claim copyright for this file I believe.
And why you adding the whole merkle-mountain-range project? we only need to make some changes in FileBasedDb I think.

get mmrPath(): string {
return this._config.mmrPath
? this._config.mmrPath
: path.join(process.cwd(), `../../mmrs/${this.subqueryName}.mmr`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
: path.join(process.cwd(), `../../mmrs/${this.subqueryName}.mmr`);
: path.join(process.cwd(), `.mmr/${this.subqueryName}.mmr`);

Copy link
Collaborator

Choose a reason for hiding this comment

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

And make sure the folder exists, otherwise mkdir first

@@ -126,6 +127,12 @@ export class NodeConfig implements IConfig {
return this._config.proofOfIndex;
}

get mmrPath(): string {
return this._config.mmrPath
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return this._config.mmrPath
return this._config.mmrPath ?? `.mmr/${this.subqueryName}.mmr`


const logger = getLogger('mmr');
const DEFAULT_WORD_SIZE = 32;
const DEFAULT_LEAF = Buffer.alloc(32);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const DEFAULT_LEAF = Buffer.alloc(32);
const DEFAULT_LEAF = Buffer.alloc(DEFAULT_WORD_SIZE);

ensureFileBasedMmr(projectMmrPath: string) {
let fileBasedDb: FileBasedDb;
if (fs.existsSync(projectMmrPath)) {
fileBasedDb = FileBasedDb.open(projectMmrPath);
Copy link
Collaborator

Choose a reason for hiding this comment

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

the flag as+ already cover the scenario that the file exists. I think the additional scenario need to be tend to is when the folder (not the file) doesn't exists.

@ianhe8x ianhe8x merged commit 6a8d04f into main Oct 14, 2021
@ianhe8x ianhe8x deleted the proof-of-index-pt2 branch October 14, 2021 04:26
bz888 pushed a commit that referenced this pull request Jun 3, 2022
* Draft

* Mmr service draft

* tidy up

* Fix

* fix mmr with forked mmr registry (experimental)

* update test for indexer.manager

* Fix lint

* Update mmr dependency

* Update mmr dependency

* change mmr dependency to @subql/x-merkle-mountain-range

* remove mmr vendor, use forked mmr repo

* Fix

* change default leaf

* Fly-by fix, remove unused import
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.

Proof of index
2 participants