btc1 / bitcoin

btc1 project bitcoin implementation
MIT License
329 stars 55 forks source link

[Consensus] Replace the stack-of-bools approach with scope counting to avoid quadratic runtime behaviour. #98

Closed christophebiocca closed 4 years ago

christophebiocca commented 7 years ago

Part of the fixes for #97.

The idea behind the change is that all ignored scopes are guaranteed to come after all non-ignored scopes. So we can replace the stack of booleans with a pair of counts + a persistent fExec.

Alternative solutions

Other approaches to solve the issue exist:

BTCD represents an array of tri-state (true, false, skipped) and OP_ELSE does nothing to skipped. This has maximum readability, but stores extraneous data (all trues precede the one false which precedes the skippeds, so most of the stack is redundant).

@SergioDemianLerner's original pseudocode derives the fExec flag each time, and uses the ignored count to cover the false conditional branch. I find it a bit confusing because ignored count == 1 is a special case with distinct behaviour.

Testing

All tests in src/test/test_bitcoin pass and I've got this running on my livenet node now (it has received 2 blocks without forking FWIW).

I'm not super familiar with the testing procedure. I looked at the fuzzer, but it doesn't actually have anything that feeds into EvalScript. Open to suggestions on how best to approach this.

Performance

I think this deserves a test in ./src/bench/bench_bitcoin but there isn't one for it yet. This would help us measure the performance benefit in both pathological/normal cases.