bcoin-org / bmultisig Goto Github PK

Tests around failed transactions and db states.

This is similar to the current bwallet-cli however would have added commands for multi-signature proposals.

Even though we have path entry in database for cosigners it is not automatically used to figure out XPUB.index, but will be convenient to expose xpub.index to the users and simplify API usage.

Related bcoin-org/bcoin#689 - This tries to solve XPUB issue for bcoin xpub Only wallets.

Each proposal has a state that is kept track of in the proposal db, once all of the users approve the proposal, it goes into the approved state, but there is no way to know that it has been broadcasted.

Steven would like the ability to specify broadcast: bool for when the final cosigner approves such that it can be broadcast then. This would update the state to broadcasted (or is confirmed better?) so that the UI can filter out proposals that have already been sent

• a way to extract signatures from the transaction.

I'd like to be able to do something like:

const { MultisigClient } = require('bmultisig');

Right now, lib/bmultisig.js exports a Plugin so for me to access the client,
I have to do something like:

const MultisigClient = require('./node_modules/bmultisig/lib/client');

I believe you would need to give the path to the specific file to import with Webpack as well

https://github.com/bcoin-org/bmultisig/blob/master/lib/http.js#L487

I see that the txs variable is never populated and will always return null in the response json.
Is there something that you intended to do with that value? I feel like it would be a good place to include coin objects for each input

Update bcoin, bcrypto and all other deps.

Currently there are not tests that cover RPC behaviour with bmultisig and what kind of issues it might cause.

we should cover them with test cases. There are several edge cases possible.

I'm getting the following error which causes requests to fail: Cannot read property 'wallets' of null

This is coming from here. Would probably either need to check if wallets is defined first, or just return wallets rather than wallets.wallets

Right now path is hardcoded into the client, no matter what is passed to options (https://github.com/bcoin-org/bmultisig/blob/master/lib/client.js#L20). It would be nice if we could setup a client to access a custom path. This is possible with the default client in bclient and useful in bPanel where we use the clients to send requests to the proxy server.

It's required that cosignerToken be truly random, so other cosigners can't guess the token.
BUT if two cosigners choose same cosigner token, it will be a conflict and authentication does not handle that correctly.

Probably after wallet is finalized we should invalidate wallet if two tokens are same.
Or add additional information on authentication to let you choose the cosigner you want to authenticate with.

@BluSyn

Add endpoints to manually unlock coins and reject proposals.

From my understanding, right now bmultisig cannot be a drop in replacement for the bwallet because it doesn't parse environment variables in the same way using bfcg

I don't think that this would be a huge change

See - https://github.com/bcoin-org/bcoin/blob/8c609c6e9e279a61349a0c3c24d66a756f582cbf/lib/node/node.js

The error received is:

TypeError: Cannot read property 'buffer' of null
    at RequestOptions.toHTTP (request-browser.js?8340:290)
    at _request (request-browser.js?8340:369)
    at request (request-browser.js?8340:409)
    at MultisigClient.request (client.js?9aa0:174)
    at MultisigClient.get (client.js?9aa0:223)
    at MultisigClient.getWallets (client.js?9995:87)

To reproduce:

$ mkdir /tmp/multisig-debug && cd /tmp/multisig-debug
$ npm init -y >/dev/null
$ npm install bmultisig
$ grep -rin tohttp node_modules | grep browser

> node_modules/brq/lib/request-browser.js:286:  toHTTP() {
> node_modules/brq/lib/request-browser.js:369:  const response = await fetch(opt.toURL(), opt.toHTTP());

$ less +286 node_modules/brq/lib/request-browser.js

  toHTTP() {
    return {
      method: this.method,
      headers: this.getHeaders(),
      body: this.body.buffer,  // bug here
      mode: 'cors',
      credentials: 'include',
      cache: 'no-cache',
      redirect: 'follow',
      referrer: 'no-referrer'
    };
  }

Digging a little deeper:

$ cd /tmp/multisig-debug/node_modules/bmultisig
$ jq .dependencies < package.json
{
  "bclient": "~0.0.2",
  "bcoin": "github:bcoin-org/bcoin#e093b2b",
  "bcrypto": "~0.2.0",
  "bdb": "~0.1.0",
  "bevent": "0.0.2",
  "blgr": "~0.0.2",
  "bmutex": "~0.0.2",
  "bstring": "~0.0.2",
  "bufio": "~0.1.0",
  "bval": "~0.0.2",
  "bweb": "~0.0.2"
}

I think that the problem lies in an old version of bclient being imported, you can see we are at version 0.1.3 on npm here - https://www.npmjs.com/package/bclient

We need to be able to do couple of things:

• export bmultisig wallet
• import bmultisig wallet
• import wallet from bwallet with additional bmultisig info

From testing with @BluSyn and @tynes

• Proposal ID should just be an auto-generated uuid internally instead of user defined
• Add an optional 'memo' fields to describe what it is for, up to 120 chars or perhaps smaller

There are couple of tests that are good to have and current tests may not cover edge case:

• chain/wallet reset
• lock/unlock inconsistencies between wallet and multisigdb
  • e.g. failed sent transaction
  • rpc calls
  • http endpoints?

API Updates

• export/import bmultisig (#61)
• Admin api for unlocking all coins and proposals (reject) (#63)
• Admin API for listing locked coins and proposals (info, debug) (#63)

Currently there is a cosignerPath field passed in on wallet join.
It does not appear this value is returned at any point.

It should return in the list of cosigners, along with their xpubs. This is useful for hardware wallets to know what the path is for each cosigner.

Additionally, I do not think it is necessary to save the entire path in the DB. The only part of the path that is "non-deterministic" is the purpose, since there are several different HD schemes (44, 49, 84, etc.), and likely more in the future.

I propose removing cosignerPath, and adding a purpose (uint) field, that is then returned with the list of cosigners.

This is my first time playing with bmultisig so I could be using it wrong...
Seeing unexpected behavior in its data directory creation.

I'm running it with --prefix= .../bcoin/regtest and it is correctly finding the wallet.conf file in that location. (ports and API keys are parsed correctly and it seems to be running fine)
but then, unlike bwallet, it creates a NEW directory regtest INSIDE its prefix directory:

Change/update applySignatures so it can have more flexible format. Currently it accepts sorted array of signatures, that correspond to each of the inputs.
We can accept map that is similar to coinview.

Also consider moving applySignature to the bcoin MTX.

During implementation I've been considering 2 major changes to how cosigner joinKey works would make the system much more robust.

1. The joinKey should be deterministically generated by the user creating the wallet, and should not be know-able by the server. This can be done by making it a signature using the creator's private key. The server can validate the joinKey with the creators' public key. In a "public wallet server" scenario, this prevents the server operator from hi-jacking the join process, or any "man-in-the-middle" scenario.

2. The joinKey should be serialized with both the key + wallet name. Currently in order for cosigners to join a wallet, they need many pieces of information: API Key, Wallet Name, Join Key. I think the joinKey should return as a serialized combination of all auth information, so the client can display or read a simple QR code to join a multisig wallet. This would also help in a "public wallet server" scenario, where the wallet name may be auto-generated.

@nodar-chkuaselidze

In order to support bcash, we need to start using bmultisig as plugin.
It will use primitives/address which will take care of the cashaddr.
And we can accept additional flag to disable several paths (e.g. get nested address)

@bucko13

We encountered a bug here where cosigners 2 of 3 approved of index 1 and 2. The cosigner at index 0 hasn't joined the wallet and we think that it is causing a bug on this line:

https://github.com/bcoin-org/bmultisig/blob/master/lib/proposaldb.js#L515

By returning early in proposal.applySignature and ignoring the !applied check, we were able to get it to work properly

Currently, seperation of bwallet and bmultisig is not supported.

Integration tests when running bmultisig separately from bwallet. Running bmultisig separately from bwallet can be useful in some cases, but have not been tested.