MetacoSA / NBitcoin

Comprehensive Bitcoin library for the .NET framework.
MIT License
1.88k stars 847 forks source link

Consider opt-in BIP148 #253

Closed nopara73 closed 7 years ago

nopara73 commented 7 years ago

I was wondering if you'd be open to consider adding opt-in BIP148 support.
For me I'd only need it at ChainBehavior, but I am not sure where else would modification be needed.

NicolasDorier commented 7 years ago

It is more complicated I think, there is new service bit.

alp-bitcoin commented 7 years ago

There is not a new service bit, you just need to ensure that a version bit is set after a certain date. I'll take a look at what I can do for a PR for this.

nopara73 commented 7 years ago

Isn't it only "if new header doesn't have bit 1 AND builds on bit 1 header", then header is refused?
I would attempt to create a PR, I'm just curious, if you'd be open to it in the first place.

NicolasDorier commented 7 years ago

NBitcoin does not know about BIP9, so nothing to change for this. I will check P2P layer change, but maybe there is nothing to do for NBitcoin.

nopara73 commented 7 years ago

Well, there definitely is, as I said I need the ChainBehaviour to support BIP148 if I want to support it in HiddenWallet.

alp-bitcoin commented 7 years ago

You don't need to know anything about BIP9, even SPV wallets can identify invalid BIP148 chains due to the header being wrong.

nopara73 commented 7 years ago

Also unrelated of BIP148 support it'd be good if one could identify if he's connected to a BIP148 node or not. I was not able to do it with the Node or NodesGroup class. However SPV wallets on NBitcoin might get unintentionally "sybil attacked" if they only connect to BIP148 nodes and the chain is shorter than the chain of legacy nodes.

dangershony commented 7 years ago

You don't necessarily have to change NBitcoin see how its done on the Stratis full node https://github.com/stratisproject/StratisBitcoinFullNode/blob/master/Stratis.Bitcoin/Consensus/ThresholdConditionCache.cs

NicolasDorier commented 7 years ago

I will have to read the BIP to understand better. Will do when back in Tokyo tomorrow.

dangershony commented 7 years ago

From the BIP specification "This BIP will cease to be active when segwit is locked-in" its a temporary BIP. I wonder why the need to support this BIP in a light wallet? its for full nodes to reject non segwit blocks before the activation period finishes (to try to get segwit activated) light wallets don't relay blocks as they can't validate.

NicolasDorier commented 7 years ago

Ah ok it is only different header version... I think as far as NBitcoin is concerned, nothing need to be done.

nopara73 commented 7 years ago

@dangershony It actually has very little to do with refusing blocks or headers, thad'd be a sybil attack on the network. What matters is what transactions individual people accept. The righ way to handle this is make it visible for the users which transactions happen on both, only on bip148 and only on legacy chain and calculate the balances accordingly.

@nicolasdorier why do you say there is no work to be done in NBitcoin? NBitcoin manages the headers and from the outside you cannot make the class bip141 compatible.

NicolasDorier commented 7 years ago

as far as NBitcoin is concerned, the block version is not checked.

nopara73 commented 7 years ago

as far as NBitcoin is concerned, the block version is not checked.

@NicolasDorier Well, yes, that's what needs to be implemented if BIP148 support is decided for. I am fine with not supporting BIP148, but I don't want this to happen for the wrong reasons.

In fact after the Core dev discussion on BIP148 one could say this bip is unlikely to get anywhere, since it will not get merged into Core, therefore no reason for NBitcoin to add support it and I am considering to settle on this argument.
But saying "NBitcoin has nothing to implement about it" is wrong reason to decide against it, because it clearly has and trying to make this argument is only dodging the issue.

NicolasDorier commented 7 years ago

Core not merging BIP148 does not mean that BIP148 is going anywhere. I think there is good chance it succeed still. NBitcoin only do anti DDOS check (PoW). Better consensus checks need to be done in full node, like stratis full node.

NicolasDorier commented 7 years ago

closing this one, I don't think there is anything to do for NBitcoin about bip148. Reopon if I am wrong.