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.

Sign up for GitHub

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

Jump to bottom

fix(ref-imp): make bitcoin logic make less calls to db #906

Merged
merged 7 commits into from
Oct 30, 2020
Merged

fix(ref-imp): make bitcoin logic make less calls to db #906

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a6576f2
fix(ref-imp): make bitcoin logic make less calls to db
isaacJChen Oct 30, 2020
983b3cf
fix(ref-imp): make bitcoin logic make less calls to db
isaacJChen Oct 30, 2020
a646001
fix(ref-imp): fix lint
isaacJChen Oct 30, 2020
eacc17a
fix(ref-imp): addess pr comments
isaacJChen Oct 30, 2020
890fcb1
fix(ref-imp): add comment
isaacJChen Oct 30, 2020
6ff727b
fix(ref-imp): add comment
isaacJChen Oct 30, 2020
6e9936a
fix(ref-imp): add comment
isaacJChen Oct 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 12 additions & 20 deletions lib/bitcoin/BitcoinProcessor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,8 @@ export default class BitcoinProcessor {

private bitcoinDataDirectory: string | undefined;

/** at least 10 blocks per page unless reaching the last block */
private static readonly pageSizeInBlocks = 10;
/** at least 100 blocks per page unless reaching the last block */
private static readonly pageSizeInBlocks = 100;

public constructor (config: IBitcoinConfig, versionModels: VersionModel[]) {
this.versionManager = new VersionManager(versionModels);
Expand Down Expand Up @@ -430,7 +430,7 @@ export default class BitcoinProcessor {
console.info(`Returning transactions since ${since ? 'block ' + TransactionNumber.getBlockNumber(since) : 'beginning'}...`);
isaacJChen marked this conversation as resolved.
Show resolved Hide resolved
// deep copy last processed block
const currentLastProcessedBlock = Object.assign({}, this.lastProcessedBlock!);
const [transactions, numOfBlocksAcquired] = await this.getTransactionsSince(since, currentLastProcessedBlock.height);
const [transactions, lastBlockSeen] = await this.getTransactionsSince(since, currentLastProcessedBlock.height);

// make sure the last processed block hasn't changed since before getting transactions
// if changed, then a block reorg happened.
Expand All @@ -439,8 +439,8 @@ export default class BitcoinProcessor {
throw new RequestError(ResponseStatus.BadRequest, SharedErrorCode.InvalidTransactionNumberOrTimeHash);
}

// if not enough blocks to fill the page then there are no more transactions
const moreTransactions = numOfBlocksAcquired >= BitcoinProcessor.pageSizeInBlocks;
// if last processed block has not been seen, then there are more transactions
const moreTransactions = lastBlockSeen < currentLastProcessedBlock.height;

return {
transactions,
Expand Down Expand Up @@ -807,19 +807,20 @@ export default class BitcoinProcessor {
}

/**
* Return transactions since transaction number and number of blocks acquired (Will get at least pageSizeInBlocks)
* Return transactions since transaction number and the last block seen
* (Will get at least 1 full block worth of data unless there is no transaction to return)
* @param since Transaction number to query since
* @param maxBlockHeight The last block height to consider included in transactions
* @returns a tuple of [transactions, numberOfBlocksContainedInTransactions]
* @returns a tuple of [transactions, lastBlockSeen]
*/
private async getTransactionsSince (since: number | undefined, maxBlockHeight: number): Promise<[TransactionModel[], number]> {
// test against undefined because 0 is falsey and this helps differenciate the behavior between 0 and undefined
let inclusiveBeginTransactionTime = since === undefined ? this.genesisBlockNumber : TransactionNumber.getBlockNumber(since);
isaacJChen marked this conversation as resolved.
Show resolved Hide resolved
isaacJChen marked this conversation as resolved.
Show resolved Hide resolved
let numOfBlocksAcquired = 0;

const transactionsToReturn: TransactionModel[] = [];

// while need more blocks and have not reached the processed block
while (numOfBlocksAcquired < BitcoinProcessor.pageSizeInBlocks && inclusiveBeginTransactionTime <= maxBlockHeight) {
while (transactionsToReturn.length === 0 && inclusiveBeginTransactionTime <= maxBlockHeight) {
const exclusiveEndTransactionTime = inclusiveBeginTransactionTime + BitcoinProcessor.pageSizeInBlocks;
let transactions: TransactionModel[] = await this.transactionStore.getTransactionsStartingFrom(
inclusiveBeginTransactionTime, exclusiveEndTransactionTime);
Expand All @@ -831,20 +832,11 @@ export default class BitcoinProcessor {
(since === undefined || transaction.transactionNumber > since);
});

numOfBlocksAcquired += BitcoinProcessor.getUniqueNumOfBlocksInTransactions(transactions);
inclusiveBeginTransactionTime = exclusiveEndTransactionTime;
transactionsToReturn.push(...transactions);
}

return [transactionsToReturn, numOfBlocksAcquired];
}

private static getUniqueNumOfBlocksInTransactions (transactions: TransactionModel[]): number {
const uniqueBlockNumbers = new Set<number>();
for (const transaction of transactions) {
uniqueBlockNumbers.add(transaction.transactionTime);
}

return uniqueBlockNumbers.size;
// the -1 makes the last seen transaction time inclusive because the variable is set to the exclusive one every loop
return [transactionsToReturn, inclusiveBeginTransactionTime - 1];
}
}
3 changes: 2 additions & 1 deletion lib/core/versions/latest/DocumentComposer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,8 @@ export default class DocumentComposer {
const allowedProperties = new Set(['action', 'ids']);
for (const property in patch) {
if (!allowedProperties.has(property)) {
throw new SidetreeError(ErrorCode.DocumentComposerUnknownPropertyInRemovePublicKeysPatch, `Unexpected property ${property} in remove-public-keys patch.`);
throw new SidetreeError(ErrorCode.DocumentComposerUnknownPropertyInRemovePublicKeysPatch,
`Unexpected property ${property} in remove-public-keys patch.`);
}
}

Expand Down
57 changes: 29 additions & 28 deletions tests/bitcoin/BitcoinProcessor.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,20 +309,20 @@ describe('BitcoinProcessor', () => {
describe('transactions', () => {
it('should get transactions since genesis capped by page size in blocks', async (done) => {
const verifyMock = spyOn(bitcoinProcessor, 'verifyBlock' as any).and.returnValue(Promise.resolve(true));
// return as many as page size
const transactions: TransactionModel[] = createTransactions(BitcoinProcessor['pageSizeInBlocks'], bitcoinProcessor['genesisBlockNumber'], true);
bitcoinProcessor['lastProcessedBlock'] = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sanity: the test is getting transaction since genesis, so shouldn't lastProcessedBlock be undefined?

Copy link
Contributor Author

@isaacJChen isaacJChen Oct 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is simulating the caller trying to get transactions since genesis (genesis to genesis + 99), and bitcoin's last Processed block is genesis + 100

height: Number.MAX_SAFE_INTEGER,
height: transactions[transactions.length - 1].transactionTime + 1,
hash: 'some hash',
previousHash: 'previous hash'
};
// return as many as page size
const transactions: TransactionModel[] = createTransactions(BitcoinProcessor['pageSizeInBlocks'], bitcoinProcessor['genesisBlockNumber'], true);
const laterThanMock = spyOn(bitcoinProcessor['transactionStore'], 'getTransactionsStartingFrom').and.callFake(() => {
const getTransactionsStartingFromSpy = spyOn(bitcoinProcessor['transactionStore'], 'getTransactionsStartingFrom').and.callFake(() => {
return Promise.resolve(transactions);
});

const actual = await bitcoinProcessor.transactions();
expect(verifyMock).toHaveBeenCalledTimes(1); // called after data was retrieved
expect(laterThanMock).toHaveBeenCalled();
expect(getTransactionsStartingFromSpy).toHaveBeenCalled();
expect(actual.moreTransactions).toBeTruthy(); // true because page size is reached
expect(actual.transactions).toEqual(transactions);
done();
Expand All @@ -338,13 +338,13 @@ describe('BitcoinProcessor', () => {
hash: 'some hash',
previousHash: 'previous hash'
};
const laterThanMock = spyOn(bitcoinProcessor['transactionStore'], 'getTransactionsStartingFrom').and.callFake(() => {
const getTransactionsStartingFromSpy = spyOn(bitcoinProcessor['transactionStore'], 'getTransactionsStartingFrom').and.callFake(() => {
return Promise.resolve(transactions);
});

const actual = await bitcoinProcessor.transactions();
expect(verifyMock).toHaveBeenCalledTimes(1);
expect(laterThanMock).toHaveBeenCalled();
expect(getTransactionsStartingFromSpy).toHaveBeenCalled();
expect(actual.moreTransactions).toBeFalsy(); // don't need more transactions because page size is not reached (last block reached)
expect(actual.transactions).toEqual(transactions.slice(0, transactions.length - 1)); // the last one should be omitted because it is processing
done();
Expand All @@ -360,14 +360,14 @@ describe('BitcoinProcessor', () => {
hash: 'some hash',
previousHash: 'previous hash'
};
const laterThanMock = spyOn(bitcoinProcessor['transactionStore'], 'getTransactionsStartingFrom').and.callFake(() => {
const getTransactionsStartingFromSpy = spyOn(bitcoinProcessor['transactionStore'], 'getTransactionsStartingFrom').and.callFake(() => {
return Promise.resolve(transactions);
});

const actual = await bitcoinProcessor.transactions();
expect(verifyMock).toHaveBeenCalledTimes(1);
expect(laterThanMock).toHaveBeenCalled();
expect(actual.moreTransactions).toBeTruthy();
expect(getTransactionsStartingFromSpy).toHaveBeenCalled();
expect(actual.moreTransactions).toBeFalsy(); // Looks at genesis to genesis + 100 but lastProcessedBlock is genesis + 99
expect(actual.transactions).toEqual(transactions);
done();
});
Expand All @@ -382,38 +382,39 @@ describe('BitcoinProcessor', () => {
hash: 'some hash',
previousHash: 'previous hash'
};
const laterThanMock = spyOn(bitcoinProcessor['transactionStore'], 'getTransactionsStartingFrom').and.callFake(() => {
const getTransactionsStartingFromSpy = spyOn(bitcoinProcessor['transactionStore'], 'getTransactionsStartingFrom').and.callFake(() => {
return Promise.resolve(transactions);
});

const actual = await bitcoinProcessor.transactions();
expect(verifyMock).toHaveBeenCalledTimes(1);
expect(laterThanMock).toHaveBeenCalled();
expect(getTransactionsStartingFromSpy).toHaveBeenCalled();
expect(actual.moreTransactions).toBeFalsy(); // no more transactions because past last block
expect(actual.transactions).toEqual([]); // no return because nothing is processed
done();
});

it('should group transactions correctly by transaction time', async (done) => {
it('should find at least 1 transaction and return', async (done) => {
const verifyMock = spyOn(bitcoinProcessor, 'verifyBlock' as any).and.returnValue(Promise.resolve(true));
// make the last transaction time genesis + 100 so it needs to call getTransactionsStartingFrom multiple times
const lastProcessedBlockHeight = bitcoinProcessor['genesisBlockNumber'] + 100;
// make the last transaction time genesis + 1000 so it needs to call getTransactionsStartingFrom multiple times
const lastProcessedBlockHeight = bitcoinProcessor['genesisBlockNumber'] + 1000;
bitcoinProcessor['lastProcessedBlock'] = {
height: lastProcessedBlockHeight,
hash: 'some hash',
previousHash: 'previous hash'
};
const laterThanMock = spyOn(bitcoinProcessor['transactionStore'], 'getTransactionsStartingFrom').and.callFake((begin) => {
return Promise.resolve(createTransactions(10, begin, false));
const getTransactionsStartingFromSpy = spyOn(bitcoinProcessor['transactionStore'], 'getTransactionsStartingFrom').and.callFake((begin) => {
if (begin === bitcoinProcessor['genesisBlockNumber'] + 500) {
return Promise.resolve(createTransactions(1, begin + 5, false));
}
return Promise.resolve([]);
});

const actual = await bitcoinProcessor.transactions();
expect(verifyMock).toHaveBeenCalledTimes(1);
expect(laterThanMock).toHaveBeenCalled();
expect(actual.moreTransactions).toBeTruthy(); // more transactions because last block returned is 90
expect(actual.transactions.length).toEqual(100); // 100 because 10 per query and called 10 times
expect(actual.transactions[99].transactionTime).toEqual(bitcoinProcessor['genesisBlockNumber'] + 90);
expect(actual.transactions[0].transactionTime).toEqual(bitcoinProcessor['genesisBlockNumber']);
expect(getTransactionsStartingFromSpy).toHaveBeenCalledTimes(6);
expect(actual.moreTransactions).toBeTruthy(); // more transactions because it didn't reach latestProcessedBlock
expect(actual.transactions.length).toEqual(1);
done();
});

Expand All @@ -424,13 +425,13 @@ describe('BitcoinProcessor', () => {
hash: 'some hash',
previousHash: 'previous hash'
};
const laterThanMock = spyOn(bitcoinProcessor['transactionStore'], 'getTransactionsStartingFrom').and.callFake(() => {
const getTransactionsStartingFromSpy = spyOn(bitcoinProcessor['transactionStore'], 'getTransactionsStartingFrom').and.callFake(() => {
return Promise.resolve([]);
});

const actual = await bitcoinProcessor.transactions();
expect(verifyMock).toHaveBeenCalledTimes(1);
expect(laterThanMock).toHaveBeenCalled();
expect(getTransactionsStartingFromSpy).toHaveBeenCalled();
expect(actual.moreTransactions).toBeFalsy();
expect(actual.transactions).toEqual([]);
done();
Expand All @@ -453,13 +454,13 @@ describe('BitcoinProcessor', () => {
return Promise.resolve(true);
});
const transactions = createTransactions(BitcoinProcessor['pageSizeInBlocks'], expectedHeight, true);
const laterThanMock = spyOn(bitcoinProcessor['transactionStore'], 'getTransactionsStartingFrom').and.callFake(() => {
const getTransactionsStartingFromSpy = spyOn(bitcoinProcessor['transactionStore'], 'getTransactionsStartingFrom').and.callFake(() => {
return Promise.resolve(transactions);
});

const actual = await bitcoinProcessor.transactions(expectedTransactionNumber, expectedHash);
expect(verifyMock).toHaveBeenCalledTimes(2);
expect(laterThanMock).toHaveBeenCalled();
expect(getTransactionsStartingFromSpy).toHaveBeenCalled();
expect(actual.moreTransactions).toBeFalsy();
transactions.shift();
expect(actual.transactions).toEqual(transactions); // the first one should be excluded because of since
Expand Down Expand Up @@ -530,10 +531,10 @@ describe('BitcoinProcessor', () => {
const expectedTransactionNumber = TransactionNumber.construct(expectedHeight, 0);
const verifyMock = spyOn(bitcoinProcessor, 'verifyBlock' as any).and.returnValue(Promise.resolve(true));
const transactions = createTransactions(BitcoinProcessor['pageSizeInBlocks'], expectedHeight, true);
const laterThanMock = spyOn(bitcoinProcessor['transactionStore'], 'getTransactionsStartingFrom').and.returnValue(Promise.resolve(transactions));
const getTransactionsStartingFromSpy = spyOn(bitcoinProcessor['transactionStore'], 'getTransactionsStartingFrom').and.returnValue(Promise.resolve(transactions));
const actual = await bitcoinProcessor.transactions(expectedTransactionNumber, expectedHash);
expect(verifyMock).toHaveBeenCalled();
expect(laterThanMock).toHaveBeenCalled();
expect(getTransactionsStartingFromSpy).toHaveBeenCalled();
expect(actual.moreTransactions).toBeTruthy();
done();
});
Expand Down