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

Change txdb zap to use layoutp #659

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Change txdb zap to use layoutp #659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
191d3e0
merged with hsd master
rozaydin Apr 6, 2021
1af3f1a
change zap to use layput.p instead of layout.m to prevent whole tx scan
rozaydin Nov 12, 2021
afe8b43
revert unnecessary changes
rozaydin Nov 12, 2021
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
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,4 @@ docker_data/
docs/reference/
node_modules/
npm-debug.log
.vscode/
Copy link
Member

Choose a reason for hiding this comment

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

I think things like this should go in a global gitignore on your own machine.

Copy link
Author

Choose a reason for hiding this comment

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

By passing a time value to the function, only transactions added to the DB within that window will be processed (not "all" txs)

I see, my bad missed the gte, lte params on getRangeHashes, My reasoning weakened ... (but pls see below)

/**
   * Get TX hashes by timestamp range.
   * @param {Number} acct
   * @param {Object} options
   * @param {Number} options.start - Start height.
   * @param {Number} options.end - End height.
   * @param {Number?} options.limit - Max number of records.
   * @param {Boolean?} options.reverse - Reverse order.
   * @returns {Promise} - Returns {@link Hash}[].
   */

  getRangeHashes(acct, options) {
    assert(typeof acct === 'number');

    if (acct !== -1)
      return this.getAccountRangeHashes(acct, options);

    const start = options.start || 0;
    const end = options.end || 0xffffffff;

    return this.bucket.keys({
      gte: layout.m.min(start),
      lte: layout.m.max(end),
      limit: options.limit,
      reverse: options.reverse,
      parse: (key) => {
        const [, hash] = layout.m.decode(key);
        return hash;
      }
    });
  }

but still the api docs for zap https://hsd-dev.org/api-docs/#zap-transactions states that:

2021-11-15_19-48-25

but here we are retrieving txes (using layout.m) without checking if they are pending or not ? Or is there a guard that prevents the zap to remove non-pending txes ?

 async zap(acct, age) {
    assert((age >>> 0) === age);

    const now = util.now();

    const txs = await this.getRange(acct, {
      start: 0,
      end: now - age
    });

    const hashes = [];

    for (const wtx of txs) {
      if (wtx.height !== -1)
        continue;

      assert(now - wtx.mtime >= age);

      this.logger.debug('Zapping TX: %x (%d)',
        wtx.tx.hash(), this.wid);

      await this.remove(wtx.hash);

      hashes.push(wtx.hash);
    }

    return hashes;
  }

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the guard is what I mention here. What we could do is migrate the DB so we store all pending TXs by time, and then just use the p index. That might be a worthwhile optimization but requires a migration or adding a new index altogether. I guess the gains depend on how much time the user enters, and how many confirmed TXs they've broadcasted in that timespan.

Copy link
Author

Choose a reason for hiding this comment

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

Yeap there is indeed a guard, let me go over your feedback ... thanks

12 changes: 8 additions & 4 deletions lib/wallet/txdb.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3178,11 +3178,15 @@ class TXDB {
assert((age >>> 0) === age);

const now = util.now();
const pendingTxs = await this.getPendingHashes(acct);

const txs = await this.getRange(acct, {
start: 0,
end: now - age
});
const txs = [];

for (const hash of pendingTxs) {
const tx = await this.getTX(hash);
assert(tx);
txs.push(tx);
}

const hashes = [];

Expand Down
24 changes: 24 additions & 0 deletions test/wallet-http-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -812,6 +812,30 @@ describe('Wallet HTTP', function() {
}
});

it('should zap eligible transactions', async () => {
// clear all pending txes
let pendingTxes = await wallet.getPending('default');
for (const pendingTx of pendingTxes) {
await wallet.abandon(pendingTx.hash);
}

const tx = await wallet.send({
outputs: [{ address: cbAddress, value: 1e4 }]
});

pendingTxes = await wallet.getPending('default');
assert(pendingTxes.length === 1);
assert(pendingTxes[0].hash === tx.hash);

await sleep(1001);

const {success} = await wallet.zap('default', 1);
assert(success === true);

pendingTxes = await wallet.getPending('default');
assert(pendingTxes.length === 0);
});

// this test creates namestate to use duing the
// next test, hold on to the name being used.
const state = {
Expand Down