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

Adds deriveaddresses rpc #1162

Merged
merged 5 commits into from
Aug 5, 2023
Merged

Adds deriveaddresses rpc #1162

merged 5 commits into from
Aug 5, 2023

Conversation

Vasu-08
Copy link
Contributor

@Vasu-08 Vasu-08 commented Jul 9, 2023

This pr implements deriveaddresses rpc.

@codecov
Copy link

codecov bot commented Jul 9, 2023

Codecov Report

Patch coverage: 93.16% and project coverage change: +0.13% 🎉

Comparison is base (014a104) 70.29% compared to head (82f8c8d) 70.42%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1162      +/-   ##
==========================================
+ Coverage   70.29%   70.42%   +0.13%     
==========================================
  Files         174      174              
  Lines       27367    27515     +148     
==========================================
+ Hits        19237    19377     +140     
- Misses       8130     8138       +8     
Files Changed Coverage Δ
lib/node/rpc.js 43.49% <83.78%> (+1.05%) ⬆️
lib/descriptor/keyprovider.js 93.83% <86.66%> (-0.19%) ⬇️
lib/descriptor/abstractdescriptor.js 92.68% <94.73%> (+0.61%) ⬆️
lib/descriptor/type/addr.js 91.42% <100.00%> (+0.51%) ⬆️
lib/descriptor/type/combo.js 93.02% <100.00%> (+3.36%) ⬆️
lib/descriptor/type/multisig.js 94.93% <100.00%> (+0.73%) ⬆️
lib/descriptor/type/pk.js 93.54% <100.00%> (+1.24%) ⬆️
lib/descriptor/type/pkh.js 94.28% <100.00%> (+1.42%) ⬆️
lib/descriptor/type/raw.js 94.28% <100.00%> (+0.16%) ⬆️
lib/descriptor/type/sh.js 93.10% <100.00%> (+0.79%) ⬆️
... and 4 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Vasu-08 Vasu-08 force-pushed the descriptor-3 branch 9 times, most recently from 3e26117 to 24da42c Compare July 16, 2023 13:33
@Vasu-08 Vasu-08 force-pushed the descriptor-3 branch 2 times, most recently from 57e8c2f to 6fe06c8 Compare July 17, 2023 20:55
lib/descriptor/keyprovider.js Outdated Show resolved Hide resolved
lib/descriptor/keyprovider.js Outdated Show resolved Hide resolved
lib/descriptor/keyprovider.js Outdated Show resolved Hide resolved
lib/descriptor/keyprovider.js Outdated Show resolved Hide resolved
lib/descriptor/keyprovider.js Outdated Show resolved Hide resolved
lib/descriptor/keyprovider.js Outdated Show resolved Hide resolved
lib/descriptor/keyprovider.js Show resolved Hide resolved
lib/descriptor/keyprovider.js Show resolved Hide resolved
lib/descriptor/keyprovider.js Show resolved Hide resolved
lib/descriptor/abstractdescriptor.js Outdated Show resolved Hide resolved

for (const script of scripts) {
const address = script.getAddress();
assert(address, 'Descriptor does not have a corresponding address');
Copy link
Collaborator

Choose a reason for hiding this comment

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

in what case would a descriptor not have an address?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In few cases of raw descriptor and for multisig descriptor.

Copy link
Collaborator

Choose a reason for hiding this comment

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

resolved

Copy link
Member

Choose a reason for hiding this comment

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

Is this because raw() and multi() get interpreted as raw output script at the top level? but if they are wrapped by sh() then they could have addresses

lib/descriptor/abstractdescriptor.js Outdated Show resolved Hide resolved
lib/descriptor/keyprovider.js Show resolved Hide resolved
lib/descriptor/keyprovider.js Show resolved Hide resolved
lib/descriptor/type/pk.js Outdated Show resolved Hide resolved
lib/descriptor/type/pkh.js Outdated Show resolved Hide resolved
lib/descriptor/type/sh.js Outdated Show resolved Hide resolved
lib/descriptor/type/wpkh.js Outdated Show resolved Hide resolved
lib/descriptor/type/wsh.js Outdated Show resolved Hide resolved
@Vasu-08 Vasu-08 force-pushed the descriptor-3 branch 2 times, most recently from 58cf024 to bbdb682 Compare July 19, 2023 07:51
Copy link
Collaborator

@theanmolsharma theanmolsharma left a comment

Choose a reason for hiding this comment

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

Address comments and mark the PR as ready for review. Great work so far!!

lib/script/script.js Show resolved Hide resolved
lib/descriptor/abstractdescriptor.js Outdated Show resolved Hide resolved
lib/descriptor/type/addr.js Outdated Show resolved Hide resolved
lib/node/rpc.js Outdated Show resolved Hide resolved
lib/node/rpc.js Outdated Show resolved Hide resolved
lib/node/rpc.js Outdated
Comment on lines 2409 to 2417
if (!Number.isInteger(low)) {
throw new RPCError(
errs.INVALID_PARAMETER, 'Range begin must be an integer'
);
}

if (!Number.isInteger(high)) {
throw new RPCError(
errs.INVALID_PARAMETER, 'Range end must be an integer'
);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this check is redundant. Validator class will ensure it is a u32int

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check is to ensure the entries in range array are valid. I think its required there.

lib/node/rpc.js Show resolved Hide resolved
lib/node/rpc.js Outdated Show resolved Hide resolved
lib/script/script.js Outdated Show resolved Hide resolved
@Vasu-08 Vasu-08 force-pushed the descriptor-3 branch 2 times, most recently from 9d24806 to 64f0741 Compare July 26, 2023 09:22
@Vasu-08 Vasu-08 marked this pull request as ready for review July 27, 2023 15:21
Copy link
Member

@pinheadmz pinheadmz left a comment

Choose a reason for hiding this comment

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

excellent work! did a first pass review, this is definitely on track


for (const provider of this.keyProviders) {
const pubkey = provider.getPublicKey(pos);
assert(pubkey, 'Cannot derive script without private keys');
Copy link
Member

Choose a reason for hiding this comment

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

not even from an xpub ?


for (const script of scripts) {
const address = script.getAddress();
assert(address, 'Descriptor does not have a corresponding address');
Copy link
Member

Choose a reason for hiding this comment

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

Is this because raw() and multi() get interpreted as raw output script at the top level? but if they are wrapped by sh() then they could have addresses

@@ -4,6 +4,8 @@
* https://github.com/bcoin-org/bcoin
*/

// script.fromMultisig(m, n, pubkeys, )
Copy link
Member

Choose a reason for hiding this comment

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

whoopsie ?

lib/node/rpc.js Outdated
@@ -2346,10 +2347,95 @@ class RPC extends RPCBase {
return result;
}

async deriveAddresses(args, help) {
if (help || args.length > 2 || args.length === 0)
throw new RPCError(errs.MISC_ERROR, 'deriveaddresses "descriptor"');
Copy link
Member

Choose a reason for hiding this comment

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

Is range an optional param? I think it should be,

Suggested change
throw new RPCError(errs.MISC_ERROR, 'deriveaddresses "descriptor"');
throw new RPCError(errs.MISC_ERROR, 'deriveaddresses "descriptor"' (range));

lib/node/rpc.js Outdated
Comment on lines 2356 to 2360
const desc = parseDescriptor(valid.str(0, ''), this.network, true);

if (desc.isRange() && args.length === 1) {
Copy link
Member

Choose a reason for hiding this comment

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

once you call new Validator() I don't think you need to use args any more. Take a look at other RPCs with optional params. I think you still extract the array argument with valid.array(1) and then can check if its null on L2358

*/

fromMultisig(m, n, keys) {
fromMultisig(m, n, keys, isSorted = true, isWitness = false) {
Copy link
Member

Choose a reason for hiding this comment

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

fromMultisig() is called from several places i.e. in account.js and node/rpc.js. Let's make sure that the new options are set correctly from all contexts and cover with a test (i.e. fail to create a 16 multisig with legacy, succeed with witness, then fail to create a 21 multisig with witness... from the existing wallet code without descriptors)

@Vasu-08 Vasu-08 force-pushed the descriptor-3 branch 3 times, most recently from b79f190 to 21bc9c0 Compare July 30, 2023 17:57
@Vasu-08 Vasu-08 requested a review from pinheadmz July 30, 2023 22:02
Comment on lines 350 to 389
for (const isSorted of [true, false]) {
for (const isWitness of [true, false]) {
for (let n = 1; n <= 20; n++) {
for (let m = 1; m <= n; m++) {
it(`should handle script generation for ${n} keys. (${m} of ${n}) (isSorted = ${isSorted}) (isWitness = ${isWitness})`, () => {
const keys = [];

for (let i = 0; i < n; i++) {
keys.push(HDPrivateKey.generate().publicKey);
}

let script;
let error;

try {
script = Script.fromMultisig(m, n, keys, isSorted, isWitness).toRaw().toString('hex');
} catch (e) {
error = e;
}

if (n > 15 && !isWitness) {
assert(error);
assert(error.message === `${n} keys not allowed inside script. Max allowed: ${isWitness ? 20 : 15}`);
} else {
if (isSorted) {
keys.sort((a, b) => a.compare(b));
}

const expectedScript = Opcode.fromInt(m).toRaw().toString('hex') // threshold
+ keys.map(key => '21' + key.toString('hex')).join('') // keys
+ Opcode.fromInt(n).toRaw().toString('hex') // number of keys
+ 'ae'; // OP_CHECKMULTISIG

assert(script === expectedScript);
}
});
}
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@pinheadmz
I wrote this test today. Wanted to know how you feel about these tests.
This is probably an overkill but it runs quickly so there is no harm in having it.

Copy link
Collaborator

@theanmolsharma theanmolsharma left a comment

Choose a reason for hiding this comment

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

Code review ACK 21bc9c0
Nice work!!


for (const isSorted of [true, false]) {
for (const isWitness of [true, false]) {
for (let n = 1; n <= 22; n++) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
for (let n = 1; n <= 22; n++) {
for (let n = 1; n <= 21; n++) {

I think 21 would suffice

@@ -1501,6 +1501,43 @@ describe('Wallet', function() {
});
}

for (const witness of [true, false]) {
it('should create witness multisig account for more than 15 keys', async () => {
const wallet = await wdb.create({
Copy link
Member

Choose a reason for hiding this comment

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

I expect this to throw right away for legacy. Kinda surprised we don't already check excessive n size in Account.fromOptions(). Do you think we should? Maybe right after this:

bcoin/lib/wallet/account.js

Lines 157 to 158 in 014a104

if (this.m < 1 || this.m > this.n)
throw new Error('m ranges between 1 and n');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes I think we can add a check here as well

Copy link
Member

@pinheadmz pinheadmz left a comment

Choose a reason for hiding this comment

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

ACK 82f8c8d

Show Signature
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK 82f8c8dc7baa160877bbe60397c3cd71f61960b8
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmTOS8wACgkQ5+KYS2KJ
yTqKcxAAjz3q0b8jINCyVFUDImcniA8SAcjn0+rULsghbvvbcZN773CMqLueK1L3
SlGqfeTUsRXhcxQtS9Rr8Xt7/1oL+fW7ldfojWBekFq1iZQUBe2VBDSayUyOtPc1
ghFylSCuixmTbeZQCCg4x78UfIqFouAcXqeLL0qZ6n/ZxR5CFrt+e+Y8Rm8IVnHT
kQjPnw+zmTXwmlASLv0UwqUTOmtbJ/mFAwZjxaFbAxMAOQLBCTVzx8QrPo5TWUH7
4KoPzcvk4VllSkL0vrAltK1I/8kcQcMvLqFWGNL/NFrYDKQssVilw+amRBw4AOvq
3VWC+zMX5Xl2D6CZV5Tnjwt4sHqucWWulpvYGMCXnenFOE8UYRNd4zRbwC7Tg4eL
Cx7Bw/uaRnIhE/GFMmU+19MM8+8X0r76C/5daiCwMw7ryb3vk9FKWSp+9LtEeqMu
8Em/EJqS0ccwq6pdok3uNKW4I1rxxXC5yJBXZQ6VvxS3AC3ycCvxTezRQo/165XJ
bu80nfIBmkwvJEAtcCNYr+fc9+LSK7urbkPO1ZeiG1BIIbrcaHYhg5bughWY/oc4
nCCNJUpUvsGx0ATaAeTeNBeQppc1BGM4uCsbBJkX8q5zL2QYPskNTuCduiNlfEHm
9pFCvMRa+rJjAAGqvh4VVWh1oQqEbVWi9H6r+4OaM8zc2L6k72I=
=s+L3
-----END PGP SIGNATURE-----

pinheadmz's public key is on keybase

Copy link
Collaborator

@theanmolsharma theanmolsharma left a comment

Choose a reason for hiding this comment

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

Approved!!
Address comments before merge!!
Great work!!

const script = this.compile();

if (!isWitness) {
assert(script.getSize() <= MAX_SCRIPT_PUSH, 'Script size is too large');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the check should be this because it will eventually get wrapped in sh

Suggested change
assert(script.getSize() <= MAX_SCRIPT_PUSH, 'Script size is too large');
assert(script.getSize() + 3 <= MAX_SCRIPT_PUSH,, 'Script size is too large');

Copy link
Collaborator

Choose a reason for hiding this comment

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

nvm, my bad

@pinheadmz pinheadmz merged commit 0c18028 into bcoin-org:master Aug 5, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants