wallet: check auction TXs for dust and null address before sending

[pinheadmz]

Closes #463 maybe a few more...

These sanity checks are already applied to regular money-sending NONE-covenant transactions in the createTX() method

However there is nothing yet to prevent a user from sending a BID with a lockup value that is below the dust threshold.

This is another issue with my least favorite line in the codebase, the "broadcast anyway" clause:

hsd/lib/node/fullnode.js

Lines 335 to 342 in a3049d5

	try {
	missing = await this.mempool.addTX(tx);
	} catch (err) {
	if (err.type === 'VerifyError' && err.score === 0) {
	this.error(err);
	this.logger.warning('Verification failed for tx: %x.', tx.hash());
	this.logger.warning('Attempting to broadcast anyway...');
	this.broadcast(tx);

As much as I want to change the "broadcast anyway" behavior we still need the extra sanity check in the wallet for SPV nodes, which can't check new TXs against mempool policy.

[pinheadmz]

also worth mentioning: the reason this hasn't come up in tests yet is because regtest allows nonstandard TXs:

hsd/lib/protocol/networks.js

Line 841 in a3049d5

regtest.requireStandard = false;

hsd/lib/mempool/mempool.js

Lines 1437 to 1440 in a3049d5

	// Non-contextual standardness checks.
	if (this.options.requireStandard) {
	const [valid, reason, score] = tx.checkStandard();

hsd/lib/primitives/tx.js

Lines 1128 to 1151 in a3049d5

	checkStandard() {
	if (this.version > policy.MAX_TX_VERSION)
	return [false, 'version', 0];
	if (this.getWeight() > policy.MAX_TX_WEIGHT)
	return [false, 'tx-size', 0];
	let nulldata = 0;
	for (const output of this.outputs) {
	if (output.address.isUnknown())
	return [false, 'address', 0];
	if (output.address.isNulldata()) {
	nulldata += 1;
	continue;
	}
	if (output.covenant.isUnknown())
	return [false, 'covenant', 0];
	if (output.isDust(policy.MIN_RELAY))
	return [false, 'dust', 0];
	}

[brandondees]

ACK

[chikeichan]

ACK

first time reviewing! no issues with the code -- also pulled down the branch and ran unit tests.

[pinheadmz]

rebase to 0dcf5e1:

address #479 (comment)

[pinheadmz]

@tynes added that await - can a brutha get an ACK?

[tynes]

What if somebody wants to provably burn some HNS? This would prevent somebody from using the API to do so. I don't see a consensus or policy rule that prevents such an output from existing.

[pinheadmz]

I think they can use a nulldata address then. This check I think is to catch user error when an address isn't specified in the output and the null value remains:

hsd/lib/primitives/address.js

Lines 99 to 112 in 940f463

	isNull() {
	if (this.hash.length === 20)
	return this.hash.equals(ZERO_HASH160);
	if (this.hash.length === 32)
	return this.hash.equals(consensus.ZERO_HASH);
	for (let i = 0; i < this.hash.length; i++) {
	if (this.hash[i] !== 0)
	return false;
	}
	return true;
	}

[tynes]

Ok, looking at the implementation of isNull, it doesn't check the address.version so this would still let somebody provably burn since its not checking Address.isNulldata

[tynes]

The same block of code is here:

hsd/lib/wallet/wallet.js

Line 3288 in 89d3ab4

if (output.isDust())

[pinheadmz]

Right - if they used nulldata (which is just address version 31) they just couldn't ALSO use a ZERO_HASH as the data part.

[tynes]

ACK 0dcf5e1

[pinheadmz]

rebase to cf6d606:

rebase on master, let's see the new github workflows run!

[coveralls]

Pull Request Test Coverage Report for Build 199648531

• 7 of 8 (87.5%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.02%) to 58.976%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/wallet/wallet.js	7	8	87.5%

Totals
Change from base Build 199629162:	0.02%
Covered Lines:	19285
Relevant Lines:	30406

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 199648531

• 7 of 8 (87.5%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.02%) to 58.978%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/wallet/wallet.js	7	8	87.5%

Totals
Change from base Build 199629162:	0.02%
Covered Lines:	19286
Relevant Lines:	30406

---

💛 - Coveralls