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

Merge upstream changes from bcoin:master #10

Merged
merged 543 commits into from
Jul 25, 2019
Merged

Conversation

OstlerDev
Copy link
Collaborator

@OstlerDev OstlerDev commented Jul 15, 2019

In order to fix a large quantity of bugs that lived in fcoin, I have gone ahead and merged the upstream work from bcoin-org/bcoin:master and tested the changes.

All changes seem to be performing admirably and fcoin seems to run faster, with a lower memory footprint, and is able to gracefully shut down. I have collected the notable changes that have been made by the bcoin team into the changelog below.

Notable Changes

  • FLO transaction signing has been fixed

  • Block difficulty retargeting has been fixed

  • bcoin - The main bcoin module has been rewritten in terms of what it exposes and how. Conventions have changed (Address instead of address). Due to the use of class syntax, something like bcoin.Address(str) is no longer possible. Please take a look at lib/bcoin.js to get a better idea of what has changed.

  • bcoin - Bcoin has been rewritten using class syntax. Objects will not be inheritable with legacy prototypal inheritance.

  • db - The way that block data is stored has changed for greater performance, efficiency, reliability and portability.

  • mempool - The mempool has had a bug fixed that caused transactions to be loaded ordered by hash order upon a restart instead of ancestory dependency order

  • wallet/http - A conflict event will be correctly emitted for all double spends.

  • wallet/http - Account balances are now indexed and will appear on account objects via the API.

  • pkg - Bcoin is once again buildable with browserify.

  • pkg - Numerous modules have been moved out to separate dependencies. For example, the lib/crypto module has been moved to the bcrypto package (and also heavily optimized even further).

  • net - Support for BIP151 and BIP150 has been dropped.

  • SIGINT handling will close the full node, spvnode and wallet.

  • Block and undo block data has been moved from LevelDB into flat files.

  • The transaction and address indexes have been moved into separate LevelDB databases.

  • The transaction has been de-duplicated, and will reduce disk usage by half for those running with txindex enabled.

  • The txindex and addrindex can now be enabled after the initial block download.

  • The addrindex has been sorted to support querying for large sets of results, and will no longer cause CPU and memory exhaustion issues.

  • Several CPU and memory exhaustion issues have been resolved with some additional arguments for querying multiple sets of results for addresses that have many transactions.

  • GET /tx/address/:address has several new arguments: after, reverse
    and limit. The after argument is a txid, for querying additional results
    after a previous result. The reverse argument will change the order that
    results are returned, the default order is oldest to latest. The limit
    argument can be used to give results back in smaller sets if necessary.

Configuration changes

  • The option for coin-cache has been removed, this setting was causing issues during the sync with out-of-memory errors and was making performance worse instead of better.

  • Wallet and Node are now separated and use different persistent configuration files. Some configuration options have moved to wallet.conf, which is stored in the prefix directory next to the existing fcoin.conf.

  • All configuration options in wallet.conf will have the wallet- prefix added implicitly. The prefix is still needed when using CLI/ENV configuration methods. The wallet now has it's own HTTP server, so options such as wallet-http-host must also be specified along with http-host if you require this setting (required for Docker networking).

  • Wallet-specific settings such as api-key and wallet-auth have moved to wallet.conf. If using CLI/ENV options, these are prefixed with wallet-. Various configuration method examples are shown below.

Example using wallet.conf:

network: testnet
wallet-auth: true
api-key: hunter2
http-host: 0.0.0.0

Example using CLI options:
./bin/node --network=testnet --http-host=0.0.0.0 --wallet-http-host=0.0.0.0 --wallet-api-key=hunter2 --wallet-wallet-auth=true

Wallet API changes

The main wallet API changes have to do with changes in the structure of the HTTP server itself. The bcoin wallet HTTP server will now always listen on it's own port. Standard ports are:

main: 7315
testnet: 17315
regtest: 17415
simnet: 17515

bucko13 and others added 30 commits November 1, 2018 12:44
matches expected outputs from hsd
bcoin browser mode can not use DNS to discover seeds, so they are hard-coded.
see: https://github.com/bcoin-org/bdns/blob/master/lib/dns-browser.js#L23
There's already plenty in `/lib/net/seeds/main.js` so this is just to get browser-nodes running on testnet.
Seeds were pulled from my own `hosts.json` file on a live testnet bcoin node
Currently coinview does not account for spent coins in the mempool,
This does not create problems because we have additional checks in
right places which detect double spends, but technically
coinview should help you detect double spent in the mempool as well.
This way it will be compatible with chain.getCoinView.

getSpentView will still return all outputs that are available
in the mempool. (This could also return spentView from indexers if
available, this method is used by `signrawtransaction`.)
docs/Configuration: SPV is command-line only
To match behavior in `node` (fullnode) if a user wants to use `bmultisig` instead of the built-in wallet.
@OstlerDev OstlerDev requested review from bitspill and orpheus July 15, 2019 22:50
@OstlerDev OstlerDev self-assigned this Jul 15, 2019
@cchrysostom
Copy link
Member

Too big to decently review the whole kit and kaboodle. The change is mostly a merge from bcoin-1.02, so we can blame them for any defects.

@OstlerDev , would you mind describing the testing you did so we can have good feels about the result of the massive PR?

@OstlerDev
Copy link
Collaborator Author

Here is the full changelog provided by the bcoin team, I will extract out the important details and place them into the main Pull Request description :)

Bcoin release notes & changelog

v2.0.0-dev

How to upgrade

The way that block data is stored has changed for greater performance,
efficiency, reliability and portability.

  • Block and undo block data has been moved from LevelDB into flat files.
  • The transaction and address indexes have been moved into separate
    LevelDB databases.
  • The transaction has been de-duplicated, and will reduce disk usage by
    half for those running with txindex enabled.
  • The txindex and addrindex can now be enabled after the initial
    block download.
  • The addrindex has been sorted to support querying for large sets
    of results, and will no longer cause CPU and memory exhaustion issues.
  • The addrindex will correctly distinguish between p2pkh and
    p2wpkh addresses.

To upgrade to the new disk layout it's necessary to move block data
from LevelDB (e.g. ~/.bcoin/chain) to a new file based block
storage (e.g. ~./.bcoin/blocks), and remove txindex and addrindex
data from the chain database, for those that have that feature enabled.

To do this you can run:

node ./migrate/chaindb4to6.js /path/to/bcoin/chain

The migration will take 1-3 hours, depending on hardware. The block data
will now be stored at /path/to/bcoin/blocks, after the data has been moved
the chain database will be compacted to free disk space.

Alternatively, you can also sync the chain again, however the above
migration will be faster as additional network bandwidth won't be used
for downloading the blocks again.

For those with txindex and addrindex enabled, the indexes will be
regenerated by rescanning the chain on next startup, this process can
take multiple hours (e.g. 8 hours) depending on hardware and the
index. Please take the potential downtime in re-indexing into account
before upgrading.

Wallet API changes

HTTP

  • PUT /wallet/:id Creating a watch-only wallet now requires an accountKey
    argument. This is to prevent bcoin from generating keys and addresses the
    user can not spend from.
  • POST /wallet/:id/create
    • Now has a sign argument for optional signing of transactions.
    • Now has a template option, that will skip templating inputs when
      sign = false, but you can enable it if necessary. It does not have an
      effect when sign = true.
    • Exposes blocks, which can will be used if there is no rate option.
    • Exposes sort (Default true), that can be used to disable BIP69 sorting.

RPC

  • Bug fix addresses for the getnewaddress command with various networks.
  • Deprecate the ismine and iswatchonly fields from the validateaddress
    command and add isscript, iswitness, ischange, witness_version
    and witness_program to partially match the v0.18.0 Bitcoin Core release
    (26a2000)
  • Add wallet RPC getaddressinfo to return ismine and iswatchonly
    with the correct values instead of their previous values which were
    hardcoded to false. Also returns address, scriptPubKey, isscript,
    iswitness, witness_version and witness_program.
    (a28ffa2)

Node API changes

HTTP

Several CPU and memory exhaustion issues have been resolved with some
additional arguments for querying multiple sets of results for addresses
that have many transactions.

  • GET /tx/address/:address has several new arguments: after, reverse
    and limit. The after argument is a txid, for querying additional results
    after a previous result. The reverse argument will change the order that
    results are returned, the default order is oldest to latest. The limit
    argument can be used to give results back in smaller sets if necessary.
  • POST /tx/address This has been deprecated, instead query for each address
    individually with GET /tx/address/:address with the expectation that
    there could be many results that would additionally need to be queried
    in a subsequent query using the after argument to request the next set.
  • POST /coin/address and GET /coin/address/:address are deprecated as
    coins can be generated using results from /tx/address/:address and
    querying by only a range of the latest transactions to stay synchronized.
    Coins could otherwise be removed from results at any point, and thus the
    entire set of results would need to be queried every time to discover
    which coins have been spent and are currently available.
  • GET / has new fields .indexes.{addr,tx} for the status of indexers.

Network changes

  • Regtest params have been updated to correspond with other bitcoin
    implementations for cross testing. Key prefixes have been updated and
    segwit is always active (39684df). The
    regtest seeds have also been cleared so that the node will not attempt
    to connect to itself.
  • Testnet seeds have been updated (8b6eba1)

Logging changes

  • Wallet and node HTTP and RPC is now more clearly identified. The wallet
    HTTP server will log with the context wallet-http and the RPC as
    wallet-rpc. The node HTTP will log with the context node-http and the
    RPC as node-rpc. There was also a reduction in the duplicate logs in
    the case of an error for the node RPC, and the addition of method logging
    for the wallet RPC.
  • There is now also additional logging during wallet rescans.

Configuration changes

  • Pool options now has a --discover option exposed, and the --only node
    option will disable discovery of new nodes.
  • The option for coin-cache has been removed, this setting was causing
    issues during the sync with out-of-memory errors and was making performance
    worse instead of better.
  • The database location for indexes can be configured via the
    --index-prefix option. Default locations are prefix + /index
    (e.g. ~/.bcoin/testnet/index/tx and ~/.bcoin/testnet/index/addr).

Script changes

  • Script has been updated for policy flags. For example
    VERIFY_CONST_SCRIPTCODE to disable OP_CODESEPARATOR in non-segwit
    scripts and make a positive findAndDelete result invalid.
  • Segwit scripts that terminate with a stack size not equal to one
    now has as error of CLEANSTACK instead of EVAL_FALSE.
  • VERIFY_UPGRADABLE_NOPS now does not apply to OP_CHECKLOCKTIMEVERIFY
    and OP_CHECKSEQUENCEVERIFY.

Testing changes

  • Switched to use a security focused rewrite of mocha called
    bmocha, for further details see: https://github.com/bcoin-org/bmocha
  • Data has been updated to be in sync with other implementations
    around policy-only script validation.
  • Tests now cleanly close with all timers being cleared.
  • Config file wallet.conf won't be read during test runs that was
    causing issues with some testing environments.

Chain changes

  • The transaction index methods are now implemented at node.txindex:
  • getMeta(hash)
  • getTX(hash)
  • hasTX(hash)
  • getSpentView(tx)
  • The address index method getHashesByAddress is now implemented
    at node.addrindex:
    • getHashesByAddress(addr) It now accepts Address instances
      rather than Address|String and the results are now sorted in
      order of appearance in the blockchain.
    • getHashesByAddress(addr, options) A new options argument has
      been added with the fields:
      • after - A transaction hash for results to begin after.
      • limit - The total number of results to return at maximum.
      • reverse - Will give results in order of latest to oldest.
  • The following methods require node.addrindex.getHashesByAddress
    in conjunction with node.txindex.getTX and node.txindex.getMeta
    respectively, and now includes a new options argument as described
    above for getHashesByAddress:
    • node.getMetaByAddress(addr, options)
    • node.getTXByAddress(addr, options)
  • The following method has been deprecated:
    • getCoinsByAddress(addr)

Other changes

  • A new module for storing block data in files.
  • Use of buffer-map for storing hashes
    (see Hashes as hex strings or buffers? bcoin-org/bcoin#533).
  • Use of bsert for assertions.
  • SIGINT handling will close the full node, spvnode and wallet.
  • Using the bcoin library in a REPL now has auto-completion by pressing tab.
  • Various documentation updates.
  • Mempool fix to add non-standard (non-segwit) to reject cache.
  • A lockfile is now included and all dependencies integrity verified
    with sha512 hash and not the vulnerable sha1 hash.
  • Updates to dependencies including bcrypto to version > 3.
  • Various small fixes to run bcoin in a browser.

v1.0.0

Migration

The latest bcoin release contains almost a total rewrite of the wallet. A
migration is required.

Bcoin has also been modularized quite a bit. Users installing globally from npm
should be sure to run:

$ npm update -g bcoin

For users who cloned the respository themselves, it might be wise to simply
remove the node_modules directory and do a fresh npm install.

$ cd bcoin/
$ git pull origin master
$ rm -rf node_modules/
$ npm install
$ npm test

Once upgrading is complete, a migration script must be executed. The migration
script resides in ./migrate/latest as an executable file.

A bcoin prefix directory must be passed as the first argument.

Example:

$ ./migrate/latest ~/.bcoin

What this does:

  • Renames chain.ldb to chain.
  • Renames spvchain.ldb to spvchain.
  • Renames walletdb.ldb to wallet.
  • Runs a migration (chaindb3to4.js) on chain.
  • Runs a migration (chaindb3to4.js) on spvchain.
  • Runs a migration (walletdb6to7.js) on wallet.

The chain migration should be minor, and only taxing if you have
--index-address enabled.

If you have a testnet, regtest, or simnet directory, the migration must also be
run on these, e.g.

$ ./migrate/latest ~/.bcoin/testnet

Also note that the --prefix argument now always creates a fully-fledged
prefix directory. For example --prefix ~/.bcoin_whatever --network testnet
will create a directory structure of ~/.bcoin_whatever/testnet/ instead of
~/.bcoin_whatever. Please update your directory structure accordingly.

Configuration changes

Wallet and Node are now separated and use different persistent configuration files.
Some configuration options have moved to wallet.conf, which is stored in the prefix
directory next to the existing bcoin.conf.

All configuration options in wallet.conf will have the wallet-
prefix added implicitly. The prefix is still needed when using CLI/ENV configuration
methods. The wallet now has it's own HTTP server, so options such as
wallet-http-host must also be specified along with http-host
if you require this setting (required for Docker networking).

Wallet-specific settings such as api-key and wallet-auth
have moved to wallet.conf. If using CLI/ENV options, these are prefixed with wallet-.
Various configuration method examples are shown below.
Note some config options (eg. network) are automatically passed to wallet plugin
if specified through CLI/ENV, but should be specified in wallet.conf for bwallet-cli.

Example using wallet.conf:

network: testnet
wallet-auth: true
api-key: hunter2
http-host: 0.0.0.0

Example using CLI options:
./bin/node --network=testnet --http-host=0.0.0.0 --wallet-http-host=0.0.0.0 --wallet-api-key=hunter2 --wallet-wallet-auth=true

Example using ENV:
BCOIN_NETWORK=simnet BCOIN_HTTP_HOST=0.0.0.0 BCOIN_WALLET_HTTP_HOST=0.0.0.0 BCOIN_WALLET_API_KEY=hunter2 BCOIN_WALLET_WALLET_AUTH=true ./bin/node

Bcoin client changes

The bcoin cli interface has been deprecated and is now replaced with a
separate tool: bcoin-cli. bcoin wallet has also been deprecated and is
replaced with bwallet-cli. Both bcoin-cli and bwallet-cli can be
installed with the bclient package in npm.

Wallet API changes

The main wallet API changes have to do with changes in the structure of the
HTTP server itself. The bcoin wallet HTTP server will now always listen on it's
own port. Standard ports are:

main: 8334
testnet: 18334
regtest: 48334
simnet: 18558

The default account object is no longer present on the wallet. To retrieve it,
request /wallet/:id/account/default instead. More minor changes to the JSON
output include the removal of id and wid properties on a number of objects.
Please update your code accordingly.

For socket.io events, all wallet prefixes have been removed. For example,
wallet join should be replaced with simply join. All wallet-related events
(tx, confirmed, etc) now receive the wallet ID as the first argument. The
tx event will receive [id, details] instead of [details].

The _admin endpoints have been moved up a level. Instead of requesting
/wallet/_admin/backup (for example), request /backup instead. If
--wallet-auth is enabled, a new flag --admin-token is accepted. Admin token
is a 64 character hex string and allows access to all admin routes. For
example, GET /backup?token=xxx.... It also allows access to any wallet on the
HTTP or websocket layer (websockets who use the admin token can join a wallet
of *, which notifies them of events on all wallets).

Notable changes

  • wallet/http - A conflict event will be correctly emitted for all
    double spends.
  • wallet/http - Account balances are now indexed and will appear on account
    objects via the API.
  • bcoin - Bcoin has been rewritten using class syntax. Objects will not be
    inheritable with legacy prototypal inheritance.
  • pkg - Bcoin is once again buildable with browserify.
  • pkg - Numerous modules have been moved out to separate dependencies. For
    example, the lib/crypto module has been moved to the bcrypto package (and
    also heavily optimized even further).
  • bcoin - The main bcoin module has been rewritten in terms of what it
    exposes and how. Conventions have changed (Address instead of address).
    Due to the use of class syntax, something like bcoin.Address(str) is no
    longer possible. Please take a look at lib/bcoin.js to get a better idea of
    what has changed.
  • net - Support for BIP151 and BIP150 has been dropped.

@OstlerDev
Copy link
Collaborator Author

To test this Pull Request I have performed the following tests

  1. Sync a full node on Testnet
  2. Sync a full node on Mainnet
  3. Send 10k transactions to full node on Testnet
  4. Integrate updated fcoin into Flosight web explorer

@OstlerDev
Copy link
Collaborator Author

@cchrysostom @orpheus @bitspill All of the changes for this upgrade are complete and this PR is ready to be merged once one of you gives approval 😃

Copy link
Collaborator

@orpheus orpheus left a comment

Choose a reason for hiding this comment

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

lot of work =O

@OstlerDev
Copy link
Collaborator Author

It was a lot of work, but totally worth it to get all the enhancements the bcoin team have been making :) I am excited for all the possibilities that an updated fcoin can enable!

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.