search

paritytech / parity-bitcoin

The Parity Bitcoin client
GNU General Public License v3.0
731 stars 215 forks source link

Proper fix for `verification::Deployments` panic #443

Open svyatonik opened 7 years ago

svyatonik commented 7 years ago

Originally code in support_segwit branch had a panic here: https://github.com/paritytech/parity-bitcoin/blob/2cacae099ead6b8240a570e4b9226e4007843be5/verification/src/deployments.rs#

This was fixed (temporarily?) by this commit: https://github.com/paritytech/parity-bitcoin/commit/9e58d50d9124c2c6ce89bed0705292ce20c387e5#diff-39fbf28d89d87ddaff46695c971ec50fR142

But: 1) looks like it is fix of side-effects, not the original reason. Probably we're checking bits for wrong period at all. 2) by @debris : this condition could possibly lead to panic in case of reorgs: https://github.com/paritytech/parity-bitcoin/blob/2cacae099ead6b8240a570e4b9226e4007843be5/verification/src/deployments.rs#L96 3) the code will currently work only if called with always-increasing block height only (previous value from cache is always used, ignoring height of block, for which it was calculated). Need to add assert over there, or change cache logic 4) if cache is empty && database (BlockHeaderProvider) contains blocks, in which some Deployment has activated/failed, this state will be returned for all previous blocks because of this: https://github.com/paritytech/parity-bitcoin/blob/2cacae099ead6b8240a570e4b9226e4007843be5/verification/src/deployments.rs#L102 5) since we create separate ChainVerifier for each block AND it takes a lot of time to calculate deployment state, it would be great to use shared cache for ChainVerifier instances. Now cache only works for single block verification.

svyatonik commented 7 years ago

(1) && (5) are fixed in #451 && #452