Optional network considered harmful

[dcousens]

Reference: https://github.com/bitcoinjs/bitcoinjs-lib/pull/321

Should the interfaces ECPair.fromWIF and HDNode.fromBase58 always require a network parameter? We could use auto-detect based on the version flag, but there comes up the problem that networks recently added to the network constants have duplicate constants that would mean auto detection may be misleading for those unaware.

In this sense, it is considered harmful in its current state, where the bitcoin default can regularly throw off users (especially with .getAddress() in <2.0.0) and the auto detect can cause incredibly difficult-to-trace bugs.

[dcousens]

@weilu @caedesvvv

[dcousens]

I want a decision on this to be made before 1.4.0 1.5.0 so a deprecation message if necessary can be added. Any change will be an API change and therefore have to be in 2.0.0.

[weilu]

https://github.com/bitcoinjs/bitcoinjs-lib/pull/321#issuecomment-65015160 If this were cryptocoinjs-lib I might have a different opinion. IMO, supporting other networks is more of a bonus so I'd rather throwing away the offending networks' constants rather than removing bitcoin as the default network

[dcousens]

@weilu how does that work with the auto detection feature then? I understand it is bitcoin js-lib, but the reality is we are supporting these features, and the support is error prone at this point.

[weilu]

With https://github.com/bitcoinjs/bitcoinjs-lib/pull/323, HDNode.fromBase58 auto-detects when network is not passed in; while ECPair.fromWIF always auto-detects as wif versions are unique across networks. What's wrong with the current state?

[caedesvvv]

+1 to having network parameter and I don't mind if it's enforced. Would be fine to deprecate autodiscovery I don't think we use it other than in unit tests and we have had issues when forgetting the paremeter.

I'm also for having support for all networks. If we want to have autodetection then it might work only for most popular networks that don't repeat certain constants.

As a user also I foresee we will support networks with conflicting parameters at some point, or people would fork our software. I see this can easily be done for any network by adding your constants to Bitcoin.networks, even if they're not originally there. Imo the library shouldn't break which means either autodetection sorts out conflicts or don't try to autodetect. I think there are different ways to do this depending where we (well you :)) want to put the easy of usage. Like you can prioritize the networks if there is a conflict, crash if several networks conflict with the same priority.

@weilu, isn't the matter that wif versions are not always unique?

[kyledrake]

IMO, supporting other networks is more of a bonus so I'd rather throwing away the offending networks' constants rather than removing bitcoin as the default network

I think I agree with @weilu here. I think it should default to mainnet, and the burden of correctly identifying the network constants should be the responsibility of the altcoin developers when there are conflicts. This library has given altcoins plenty of rope, but altcoins really shouldn't be sharing constants with mainnet, I'm not really an expert on this, but it feels a little sloppy that some of them are. I would hate to increase the interface burden on the majority of this library's users because of altcoin fork constant collisions with mainnet.

That said, @caedesvvv being okay with it softens my position on it quite a bit.

[dcousens]

@kyledrake, you're commenting as if non bitcoin networks are the only problematic scenario.

In reality, a typical codebase swaps between testnet and mainnet interchangeably, and this 'feature' ends up biting you several times over. On Dec 3, 2014 6:39 PM, "Kyle Drake" notifications@github.com wrote:

IMO, supporting other networks is more of a bonus so I'd rather throwing away the offending networks' constants rather than removing bitcoin as the default network

I think I agree with @weilu https://github.com/weilu here. I think it should default to mainnet, and the burden of correctly identifying the network constants should be the responsibility of the altcoin developers when there are conflicts. This library has given altcoins plenty of rope, but altcoins really shouldn't be sharing constants with mainnet, I'm not really an expert on this, but it feels a little sloppy that some of them are. I would hate to increase the interface burden on the majority of this library's users because of altcoin fork constant collisions with mainnet.

That said, @caedesvvv https://github.com/caedesvvv being okay with it softens my position on it quite a bit.

— Reply to this email directly or view it on GitHub https://github.com/bitcoinjs/bitcoinjs-lib/issues/325#issuecomment-65366344 .

[dcousens]

There is two API issues being discussed here:

• optional network parameter when network information may already be present, and thus, detected
• optional network parameter that defaults to Bitcoin

I vote we definitely remove any cases of the latter. The former, I'm still formulating an opinion on. On Dec 3, 2014 6:52 PM, "Daniel Cousens" wrote:

@kyledrake, you're commenting as if non bitcoin networks are the only problematic scenario.

In reality, a typical codebase swaps between testnet and mainnet interchangeably, and this 'feature' ends up biting you several times over. On Dec 3, 2014 6:39 PM, "Kyle Drake" notifications@github.com wrote:

IMO, supporting other networks is more of a bonus so I'd rather throwing away the offending networks' constants rather than removing bitcoin as the default network

I think I agree with @weilu https://github.com/weilu here. I think it should default to mainnet, and the burden of correctly identifying the network constants should be the responsibility of the altcoin developers when there are conflicts. This library has given altcoins plenty of rope, but altcoins really shouldn't be sharing constants with mainnet, I'm not really an expert on this, but it feels a little sloppy that some of them are. I would hate to increase the interface burden on the majority of this library's users because of altcoin fork constant collisions with mainnet.

That said, @caedesvvv https://github.com/caedesvvv being okay with it softens my position on it quite a bit.

— Reply to this email directly or view it on GitHub https://github.com/bitcoinjs/bitcoinjs-lib/issues/325#issuecomment-65366344 .

[kyledrake]

In reality, a typical codebase swaps between testnet and mainnet interchangeably, and this 'feature' ends up biting you several times over.

@dcousens fair point.

[dcousens]

My conclusion here was to remove optional network parameter(s) that default to Bitcoin, and to provide an optional network parameter when network information may already be present that a user can use to force a certain network.

As for #278, I think we should simply add instructions for how to add your own network information easily (see below), and then remove any networks not in the top 5 by market cap.

var networks = require('bitcoinjs-lib').networks

networks.viacoin = {
  magicPrefix: '\x18Viacoin Signed Message:\n',
  bip32: {
    public: 0x0488b21e,
    private: 0x0488ade4
  },
  pubKeyHash: 0x47,
  scriptHash: 0x21,
  wif: 0xc7,
  dustThreshold: 560,
  dustSoftThreshold: 100000,
  feePerKb: 100000
}

With estimateFee being moved to Transaction.

[weilu]

Works for me. We should make it clear in our docs that the additional networks should be defined before other function calls to bitcoinjs-lib, alongside what are compulsory/optional fields for a network object.

[dcousens]

I vote we remove estimateFee entirely, it is misleading, and with the introduction of Transaction.prototype.byteLength, you can just do the Math.ceil(byteLength / 1000) * feePerKb very easily yourself.

Given feePerKb is now a protocol level value (see https://bitcoin.org/en/developer-reference#estimatefee), that is, non-constant, I think this makes more sense.

This application level complexity isn't something that should be hidden away.

[jprichardson]

I vote we remove estimateFee entirely, it is misleading, and with the introduction of Transaction.prototype.byteLength, you can just do the Math.ceil(byteLength / 1000) * feePerKb very easily yourself.

Seems reasonable to me.

[dcousens]

The remaining part of this issue should be resolved with the merging of ECPair.

edit: Actually, we also need to remove all the bitcoin defaults.