P2SH tx test added. Max sigops count per txn is 20k.
Details
Details
- Reviewers
deadalnix - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) Restricted Project - Commits
- rSTAGING207f2e1281cc: testing bad-txn-sigops using p2sh
rABC207f2e1281cc: testing bad-txn-sigops using p2sh
../qa/rpc-tests/abc-p2p-fullblocktest.py
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
This mostly looks good. A few change request, but nothing terrible, just going from good to great.
| qa/rpc-tests/abc-p2p-fullblocktest.py | ||
|---|---|---|
| 333 ↗ | (On Diff #501) | You don't need to create a local here. Especially since this local have a very generic name, so the same name risk being reused. |
| 335 ↗ | (On Diff #501) | Don't bother computing specific value here. Just spend 1 and send the rest as miner's fee. |
| 345 ↗ | (On Diff #501) | You don't need to assert this. If the block structure come to change for whatever reason, this test will break, while it has nothing to do with what is being tested. |
| 361 ↗ | (On Diff #501) | Please check the very limit. This should be MAX_BLOCK_SIGOPS_PER_MB - redeem_script.GetSigOpCount(True) Because you reuse that value down bellow, it make sense to store it in a local variable. |
| 362 ↗ | (On Diff #501) | ONE_MEGABYTE + 1 to make it explicit it goes over 1MB. This is the case because you add a tx, but at least this'll be explicit. |
| 363 ↗ | (On Diff #501) | space after coma. |
| 378 ↗ | (On Diff #501) | Please move the check after that one so it doesn't get modified, plus simple test case coming before more complex ones is generally a good idea. |
Comment Actions
A few more details to nail down.
| qa/rpc-tests/abc-p2p-fullblocktest.py | ||
|---|---|---|
| 343 ↗ | (On Diff #504) | Why didn't you use create_and_sign_transaction ? |
| 347 ↗ | (On Diff #504) | Assigning to a local isn't necessary anymore. |
| 354 ↗ | (On Diff #504) | This depends on block 30's layout. You already have a reference to that tx, use that. |
| qa/rpc-tests/abc-p2p-fullblocktest.py | ||
|---|---|---|
| 343 ↗ | (On Diff #504) | I didn't realized that the create_and_sing_transaction method was already defined, I'll use it. |
| 354 ↗ | (On Diff #504) | I thought it was simpler to understand what the code does if the transaction to spend was the first one of the last block. But I'll change it to spend_p2sh_tx, that way it's clear what transaction is going to be spent. |
Comment Actions
Using create_and_sign_transaction method to create the p2sh_tx
Removed b30 local variable
Using p2sh_tx variable instead of the first block transaction in the spend_p2sh_tx method
