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

Change txdb zap to use layoutp #659

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

Conversation

rozaydin
Copy link

This PR changes the txdb zap method, instead of retrieving records from layout.m (which iterates over all txes i assume), it retrieves from layout.p (pending txes) to prevent unnecessary looping over all txs.

My understanding might be wrong, I am not quite sure about layout.m if it's used to store all txes with a timestamp or not. (if there is some documentation about leveldb layout for hsd that would be awesome)
However if it makes sense, i would like to suggest the zap method to return (instead of just success: true) the list of txes it zapped. Which i can add to this PR if it makes sense ...

  • p[hash] -> dummy (pending flag)
  • m[time][hash] -> dummy (tx by time)

If you are on it, would be great to know what should be the relation between the mempool and zapped/abondaned txes

Thanks for the amazing software you are building ...

Signed-off-by: rozaydin <ridvan@namebase.io>
@pinheadmz
Copy link
Member

Hey thanks for the contribution and for looking in to this. I'm not sure this is an improvement actually. The existing zap() function uses the m index in the DB which is transactions ordered by TIME, which is the only parameter accepted by the API call. The p index is ALL pending transactions ordered by txid (so, random). By passing a time value to the function, only transactions added to the DB within that window will be processed (not "all" txs).

I couldn't tell you why the "zap" function is constrained by TX age but I think instead of breaking it, if you want to ADD a "delete all pending txs" API using the method you started implementing here, that might make more sense for a pull request.

@@ -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

@pinheadmz
Copy link
Member

I think if you continue working on this with the p index, you probably can remove this filter:

hsd/lib/wallet/txdb.js

Lines 3194 to 3195 in afe8b43

if (wtx.height !== -1)
continue;

@rozaydin rozaydin changed the title Ridvan change txdb zap to use layoutp Change txdb zap to use layoutp Nov 15, 2021
@coveralls
Copy link

coveralls commented Nov 15, 2021

Pull Request Test Coverage Report for Build 1454650398

  • 6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.7%) to 62.361%

Totals Coverage Status
Change from base Build 1391687210: 0.7%
Covered Lines: 21238
Relevant Lines: 31825

💛 - Coveralls

@rozaydin rozaydin marked this pull request as draft November 17, 2021 10:32
@nodech nodech added intermediate review difficulty - intermediate wallet-db part of the codebase wallet-http part of the codebase bug general - Something isn't working needs tests modifier labels Nov 18, 2021
@nodech nodech added this to the hsd 7.0.0 milestone Aug 16, 2023
@nodech nodech mentioned this pull request Mar 4, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug general - Something isn't working intermediate review difficulty - intermediate needs tests modifier wallet-db part of the codebase wallet-http part of the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants