Page MenuHomePhabricator

Set Monolith opcode enable script flag if monolith enabled
AbandonedPublic

Authored by deadalnix on Mar 13 2018, 12:14.

Details

Reviewers
danconnolly
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Maniphest Tasks
T299: Re-enable Satoshi OpCodes
Summary

Sets the SCRIPT_ENABLE_OPCODE_MONOLITH flag if monolith has been enabled

Co-authored-by: Joshua Yabut <yabut.joshua@gmail.com>
Co-authored-by: Marcos Mayorga <mm@mm-studios.com>
Co-authored-by: Daniel Connolly <daniel@dconnolly.com>

Test Plan

tested in test/validation_tests.cpp
and functional tests monolith-opcodes.py

make check
test/functional/monolith-opcodes.py

Diff Detail

Repository
rABC Bitcoin ABC
Branch
d1194-testing
Lint
Lint Skipped
Unit
No Test Coverage
Build Status
Buildable 2201
Build 2546: Bitcoin ABC Buildbot (legacy)
Build 2545: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

This actually do not test at all if the feature activates when you expect it to. (also, this break reorg across the activation and will cause nodes to ban each others, which a proper test would have detected).

src/test/validation_tests.cpp
3 ↗(On Diff #3168)

Just extend the Bitcoin developers's copyright

src/validation.cpp
4 ↗(On Diff #3168)

dito

This revision now requires changes to proceed.Mar 14 2018, 10:27

We'll need an RPC test that creates a block with the new opcodes in script. It should have:

  1. Create a block with transactions containing the new opcodes that are valid under new consensus rules.
  2. The activation set at some block
  3. Mine empty blocks up to that block, but no further, submit the block to activate Monolith. It should *FAIL* to be accepted.
  4. Mine a few more empty blocks to actually activate Monolith.
  5. Adjust the previously invalid block so that it would be connected to the new chain tip.
  6. Submit the block, it should *SUCCEED* at being accepted as the new chain tip.

There should be a similar test for the mempool. These tests can be interleaved with the above test to ensure that transactions are being accepted into the node's mempool appropriately as well.

FINALLY, there should be a test that:

  1. Mines some blocks.
  2. Mines a few more blocks with MTPs to activate Monolith.
  3. Submit some transactions to the mempool with the new opcodes that are valid.
  4. Mine blocks with enough work to cause a reorg, but the MTPs should be such that Monolith would now be considered INACTIVE.
  5. Finally, assert that the mempool has been cleared in this situation.

In conclusion, this should have 3 RPC tests:

  1. Check that txs within blocks are not accepted until the hardfork has been activated (and not within the block which would cause it to be activated)
  2. Check that transactions are accepted appropriately in the mempool only after Monolith has been activated.
  3. Check that a reorg shortly after activation clears the mempool of txns with new opcodes. This ensures invalid blocks will not be mined on top.
src/test/validation_tests.cpp
85 ↗(On Diff #3168)

This can be removed

add functional tests, which are failing - flags not set properly when CScriptCheck:operator() in validation.cpp calls VerifyScript()

  • fix non-activation of monolith script flag in AcceptToMemoryPoolWorker()
  • chain reorganization fixes
  • banscore test

This diff does not fix T288, the tests work around it.

src/validation.cpp
1857 ↗(On Diff #3244)

Rename this to:

GetScriptFlags which uses pindex directly.

Then make two small wrapper functions:

GetMempoolScriptFlags and
GetBlockScriptFlags

they should wrap that with the appropriate index.

198 ↗(On Diff #3215)

nit: This should be config first.

src/validation.cpp
975 ↗(On Diff #3244)

why isn't this just scriptVerifyFlags |= currentBlockScriptVerifyFlags

Just a quick note: I'm not sure that the banscore test is valid. I want to split this diff in two, with the tests defined first to confirm behaviour before the changes.

src/validation.cpp
1896 ↗(On Diff #3244)

If we fix the off-by-one this might fail IBD huh?

src/validation.cpp
1896 ↗(On Diff #3244)

ooh, good point. the issue was with accepting tx into the mempool, it wouldnt start accepting tx into the mempool until one block after the HF. Block acceptance was not affected by the issue. The DAA didnt affect the tx format, only the block format, so it should have no affect. I'm not sure how we can test this properly though, apart from doing a complete sync.

danconnolly edited the test plan for this revision. (Show Details)

fix T288 and misc small issues

src/validation.cpp
1896 ↗(On Diff #3244)

I'm running complete syncs off master every 6 hours. If we merge it, and it fails we will know.

src/validation.cpp
1870 ↗(On Diff #3279)

I'm not sure about the -1's here. It should follow the same rule as mempool vs the other scriptflag fetching. Right?

1875 ↗(On Diff #3279)

Ditto

1911 ↗(On Diff #3279)

I don't understand why blocktine or height needs to vary differently than the block that was passed in. Can you provide the rationale?

My understanding is:

  1. If you are validating TXNs in the mempool use chaintip.
  2. If you are validating a block, use previous block information.

If these are the rules, then nothing else should vary differently. This changes some behavior for old blocks, but the activation blocks should never have been able to be created with transactions that would fail such a rule above. So, we should try it and run an IBD. (It is okay to change the historic behavior as long as it won't invalidate the existing blocks)

If this is true, the blocktime parameter should go away.

test/functional/monolith-opcodes.py
201 ↗(On Diff #3279)

Comment is wrong?

src/validation.cpp
975 ↗(On Diff #3279)

Can we drop the branch and just use:

scriptVerifyFlags |= currentBlockScriptVerifyFlags

Would this change behavior?

deadalnix requested changes to this revision.Mar 22 2018, 01:24

This diff is not up to date with numerous changes done in master.

src/validation.cpp
964 ↗(On Diff #3279)

there is no reason that this would use a different function than the one connecting blocks.

1856 ↗(On Diff #3279)

This is very much outside the scope of this diff. it is very risky, and overall not very much tested.

This revision now requires changes to proceed.Mar 22 2018, 01:24
src/validation.cpp
1911 ↗(On Diff #3279)

There are several questions here about the fix to T288, but rather than address them directly, I think that we should split the fix to T288 out into its own diff, starting with a full set of unit tests for the (current) GetBlockScriptFlags() function, followed by the change to fix T288. The tests should include all "transition points" where the script flags will change and a test which demonstrates the bug. The problem with this approach is that it will take time.

src/validation.cpp
1911 ↗(On Diff #3279)

That sounds like a solid idea to me.

movrcx added inline comments.
src/validation.cpp
964 ↗(On Diff #3279)

Not sure what I'm supposed to do here... 🤔

975 ↗(On Diff #3279)

Works fine. This change will be submitted in the next diff.

user@debian:~/code/bitcoin-abc$ test/functional/test_runner.py monolith-opcodes.py
Temporary test directory at /tmp/bitcoin_test_runner_20180328_185347
........
monolith-opcodes.py passed, Duration: 4 s

TEST                | STATUS    | DURATION

monolith-opcodes.py | ✓ Passed  | 4 s

ALL                 | ✓ Passed  | 4 s (accumulated) 
Runtime: 4 s

user@debian:~/code/bitcoin-abc$ src/test/test_bitcoin
Running 294 test cases...

*** No errors detected
user@debian:~/code/bitcoin-abc$
src/validation.cpp
1555

The comment for this branch isn't matching up with the current ban score so it'll need to be changed. What's the intended ban score?

src/validation.cpp
1555

Actually nevermind.. I think this impacts mostly ConnectBlock() as the p2p transaction relay tests are running appropriately.

Rebased to fresh master.

Remove whitespace nits.

user@debian:~/code/bitcoin-abc$ test/functional/test_runner.py 
ALL                              | ✓ Passed  | 480 s (accumulated) 
...
Runtime: 127 s
user@debian:~/code/bitcoin-abc$ src/test/test_bitcoin
Running 285 test cases...

*** No errors detected

Update comment in monolith-opcodes.py

deadalnix requested changes to this revision.Mar 31 2018, 11:41
deadalnix added inline comments.
src/test/validation_tests.cpp
108 ↗(On Diff #3347)

This test doesn't test much, in addition it'll break in may.

src/validation.cpp
1517 ↗(On Diff #3347)

Make flags constant.

1571 ↗(On Diff #3347)

I don't think this is checking what you think it checks. That also means this is not tested, so there are missing test cases.

1577 ↗(On Diff #3347)

You are modifying flags here. This is a big no.

1957 ↗(On Diff #3347)

Comment.

test/functional/monolith-opcodes.py
1 ↗(On Diff #3347)

This file do not follow naming convention. You are not testing the opcodes.

2 ↗(On Diff #3347)

I had no idea core devs worked on this in 2015.

280 ↗(On Diff #3347)

Where is the code doing this ?

This revision now requires changes to proceed.Mar 31 2018, 11:41
deadalnix edited reviewers, added: danconnolly; removed: deadalnix.

Superseeded by D1231 .

This contains an off by one error for the activation, did not clean the mempool properly, and various other bugs, which in turn show that the tests are inadequate.