Just extend the Bitcoin developers's copyright

dito

This can be removed

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.

nit: This should be config first.

why isn't this just scriptVerifyFlags |= currentBlockScriptVerifyFlags

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

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.

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

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

Ditto

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.

Comment is wrong?

Can we drop the branch and just use:

scriptVerifyFlags |= currentBlockScriptVerifyFlags

Would this change behavior?

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.

That sounds like a solid idea to me.

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

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$

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?

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