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

batch open transaction support on full node #34

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
2 changes: 1 addition & 1 deletion lib/dns/resource.js
Original file line number Diff line number Diff line change
Expand Up @@ -880,7 +880,7 @@ function typeToClass(type) {
function stringToClass(type) {
assert(typeof type === 'string');

if (!hsTypes.hasOwnProperty(type))
if (!Object.prototype.hasOwnProperty.call(hsTypes, type))

Choose a reason for hiding this comment

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

I think we want to avoid directly accessing prototypes, is your eslint config using some global rules defined elsewhere?

FWIW to avoid doing x.hasOwnProperty(y), you can do: Object.keys(x).includes(y) for better safety

return null;

return typeToClass(hsTypes[type]);
Expand Down
2 changes: 1 addition & 1 deletion lib/primitives/airdropkey.js
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ class AirdropKey extends bio.Struct {
fromJSON(json) {
assert(json && typeof json === 'object');
assert(typeof json.type === 'string');
assert(keyTypes.hasOwnProperty(json.type));
assert(Object.prototype.hasOwnProperty.call(keyTypes, json.type));

Choose a reason for hiding this comment

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

Same comment


this.type = keyTypes[json.type];

Expand Down
35 changes: 35 additions & 0 deletions lib/wallet/http.js
Original file line number Diff line number Diff line change
Expand Up @@ -1035,6 +1035,41 @@ class HTTP extends Server {
return res.json(200, mtx.getJSON(this.network));
});

// Create Batch Open
this.post('/wallet/:id/batch/open', async (req, res) => {
const valid = Validator.fromRequest(req);
const names = valid.array('names');
const force = valid.bool('force', false);
const passphrase = valid.str('passphrase');
const broadcast = valid.bool('broadcast', true);
const sign = valid.bool('sign', true);
const MAX_NAME_ARRAY_LENGTH = 200;

assert(names && names.length > 0, 'Names are required.');
assert(names.length <= MAX_NAME_ARRAY_LENGTH,
`Names array shoud not exceed ${MAX_NAME_ARRAY_LENGTH}`);
assert(broadcast ? sign : true, 'Must sign when broadcasting.');

const options = TransactionOptions.fromValidator(valid);
const {mtx, errors, isAllError} = await req.wallet
.createBatchOpen(names, force, options);

Choose a reason for hiding this comment

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

This indentation does not seem correct, what eslint rules are you using?


if (isAllError) { // no valid output in mtx

Choose a reason for hiding this comment

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

Personally I don't like how hsd eslint rules want:

if (isAllError)
  return res.json(500, { errors: errors });

I think this --^ is very prone to bugs and don't like it, I prefer what you wrote, but, it's the convention in this repo, so could you change this if statement to that format? This should happen automatically with the rest of the eslint fixes in this PR

return res.json(500, {errors: errors});
}

if (broadcast) {
const tx = await req.wallet.sendMTX(mtx, passphrase);
return res.json(200, {tx: tx.getJSON(this.network), errors: errors});
}

if (sign) {
await req.wallet.sign(mtx, passphrase);
}

return res.json(200, {tx: mtx.getJSON(this.network), errors: errors});
});

// Create Bid
this.post('/wallet/:id/bid', async (req, res) => {
const valid = Validator.fromRequest(req);
Expand Down
30 changes: 30 additions & 0 deletions lib/wallet/txdb.js
Original file line number Diff line number Diff line change
Expand Up @@ -845,6 +845,36 @@ class TXDB {
return false;
}

/**
* Test whether the covenant
* has a duplicate open.
* @param {Covenant}
* @returns {Boolean}
*/

async isCovenantDoubleOpen(covenant) {

Choose a reason for hiding this comment

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

Few comments:

  1. It seems like this is copied from isDoubleOpen(tx), correct? Could you rewrite isDoubleOpen(tx) so that it uses this isCovenantDoubleOpen(covenant) function?
  2. Reading this code actually makes me think that there is a bug with the original isDoubleOpen that you copied it from! In the original, we return false from inside the for loop if the duplicate open is from an old auction. But that doesn't mean the whole transaction doesn't have a double open, just that covenant isn't! It should continue and not return false.

We can rewrite isDoubleOpen as something like:

async isDoubleOpen(tx) {
  const isDoubleOpenList = await Promise.all(
    tx.outputs.map(({ covenant }) => isCovenantDoubleOpen(covenant))
  );
  // any covenant being a double open means the whole transaction includes a double open
  return isDoubleOpenList.some(Boolean);
}

Which fixes the continue bug.

  1. Reviewing this code also caused me to review removeDoubleOpen(tx). Handshake used to have a bug that prevented valid double opens (when auction expires). isDoubleOpen fixed that bug, but removeDoubleOpen never did! It seems you're not using this removeDoubleOpen function anyway so no change necessary in this PR, just pointing it out.

Copy link
Author

Choose a reason for hiding this comment

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

removeDoubleOpen will keep it in mind, i am refactoring as you have indicated. I didnt realize the "bug" you mentioned above tough, addressing that one as well.

if (!covenant.isOpen()) {
return false;
} else {
const nameHash = covenant.getHash(0);
const key = layout.o.encode(nameHash);
const hash = await this.bucket.get(key);

// Allow a double open if previous auction period has expired
// this is not a complete check for name availability or status!
if (hash) {
const names = this.wdb.network.names;
const period = names.biddingPeriod + names.revealPeriod;
const oldTX = await this.getTX(hash);
if ( oldTX.height !== -1 && oldTX.height + period < this.wdb.height) {
return false;
}
return true;
}
}
return false;
}

/**
* Remove duplicate opens.
* @private
Expand Down
120 changes: 120 additions & 0 deletions lib/wallet/wallet.js
Original file line number Diff line number Diff line change
Expand Up @@ -1559,6 +1559,87 @@ class Wallet extends EventEmitter {
}
}

/**
* Make a batch open MTX.
* @param {Array} names

Choose a reason for hiding this comment

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

Can you replace with string[] or Array<string> instead

* @param {Number|String} acct
* @param {Boolean} force
* @returns {MTX}
*/

async makeBatchOpen(names, force, acct) {

Choose a reason for hiding this comment

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

Can you rewrite all the other makeOpen/createOpen etc calls so that they are a special case of xxxBatchOpen? I want to avoid having bug fixes in one not translate to bug fixes in the other. Perhaps something like:

async makeOpen(name, force, acct) {
  const { mtx, errors, isAllError } = await makeBatchOpen([name], force, acct);
  if (errors.length > 0) {
    throw new Error(errors[0].error);
  }

  return mtx;
}

// TODO roza - verify this method is supported
assert(Array.isArray(names));
assert(typeof force === 'boolean');
assert(acct >>> 0 === acct || typeof acct === 'string');

const errors = [];
const mtx = new MTX();

for (const name of names) {
if (!rules.verifyName(name)) {
errors.push({name: name, error: 'Invalid name.'});

Choose a reason for hiding this comment

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

The key error makes this sound like a typed object, could you call it message or errorMessage?

continue;
}

const rawName = Buffer.from(name, 'ascii');
const nameHash = rules.hashName(rawName);
const height = this.wdb.height + 1;
const network = this.network;

// TODO: Handle expired behavior.
if (rules.isReserved(nameHash, height, network)) {
errors.push({name: name, error: 'Name is reserved.'});
continue;
}

if (!rules.hasRollout(nameHash, height, network)) {
errors.push({name: name, error: 'Name not yet available.'});
continue;
}

let ns = await this.getNameState(nameHash);
if (!ns) {
ns = await this.wdb.getNameStatus(nameHash);
}

ns.maybeExpire(height, network);

const state = ns.state(height, network);
const start = ns.height;

if (state !== states.OPENING) {
errors.push({name: name, error: 'Name is not available.'});
continue;
}

if (start !== 0 && start !== height) {
errors.push({name: name, error: 'Name is already opening.'});
continue;
}

const addr = await this.receiveAddress(acct);

const output = new Output();
output.address = addr;
output.value = 0;
output.covenant.type = types.OPEN;
output.covenant.pushHash(nameHash);
output.covenant.pushU32(0);
output.covenant.push(rawName);

if (await this.txdb.isCovenantDoubleOpen(output.covenant)) {
errors.push({name: name, error: `Already sent an open for: ${name}.`});
continue;
}

mtx.outputs.push(output);
}

const isAllError = (names.length === errors.length);
return { mtx: mtx, errors: errors, isAllError: isAllError};
}

/**
* Make a open MTX.
* @param {String} name
Expand Down Expand Up @@ -1656,6 +1737,45 @@ class Wallet extends EventEmitter {
}
}

/**
* Create and finalize a batch open
* MTX without a lock.
* @param {Array} names
* @param {Boolean} force
* @param {Object} options
* @returns {MTX}
*/

async _createBatchOpen(names, force, options) {
const acct = options ? options.account || 0 : 0;
const {mtx, errors, isAllError} = await this
.makeBatchOpen(names, force, acct);
if (!isAllError) {
await this.fill(mtx, options);
return {mtx: await this.finalize(mtx, options), errors: errors};
} else {
return {mtx: null, errors: errors, isAllError: true};
}
}

/**
* Create and finalize a batch open
* MTX with a lock.
* @param {Array} names
* @param {Boolean} force
* @param {Object} options
* @returns {MTX}
*/

async createBatchOpen(names, force, options) {
const unlock = await this.fundLock.lock();
try {
return await this._createBatchOpen(names, force, options);
} finally {
unlock();
}
}

/**
* Create and send an open
* MTX without a lock.
Expand Down
Loading