test: Add test for BIP30 duplicate tx #16363
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Coverage
Updated at: 2019-07-09T23:07:22.556814. |
|
Concept ACK |
I think we cannot add such a test because it would violate consensus rules, except for the two hardcoded exceptions on mainnet! Also I think it's worth pointing out that the optimizations we have in place for mainnet, where we don't actually enforce BIP30 explicitly if we're on the BIP34-activating chain we expect to be on, makes regtest-testing less meaningful here. However, we do rely on BIP 30 being explicitly enforced for alternate chains, which is critical to allowing a node to reorg to the main chain if being fed an alternate early history, so this is probably a good test to add overall. |
Yes, we'd have to add a regtest exception as well, and I'd prefer not to modify source code in a pull that only adds tests. |
|
We check that the hardcoded exceptions work correctly everytime someone does an IBD and we check the non-exceptional case every other block, so doesn't seem like there's much value in adding regtest-only exceptions to me... |
|
Code review ACK fa8489a |
maflcko
pushed a commit
to maflcko/bitcoin-core
that referenced
this pull request
Aug 5, 2019
fa8489a test: Add test for BIP30 duplicate tx (MarcoFalke) 77770d9 test: Properly serialize BIP34 coinbase height (MarcoFalke) Pull request description: This adds a test for BIP30 to check that duplicate txs can exist in the blockchain given the first one was completely spent when the second one is added. (Requested by ajtowns in bitcoin#16333 (comment)) We can not add a test that a later duplicate tx overwrites a previous one, because BIP30 is always enforced on regtest. If someone feels strongly about such a test, some Bitcoin Core code would have to be modified, which can be done in a follow up pull request. Also, add a commit to fix the BIP34 test failures reported in bitcoin#14633 (comment) ACKs for top commit: laanwj: Code review ACK fa8489a Tree-SHA512: c707d0bdc93937263876b603425b53322a2a9f9ec3f50716ae2fa9de8ddc644beb22b26c1bfde7f4aab102633e096b354ef380db919176bd2cb44a2828f884aa
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Aug 10, 2019
fa8489a test: Add test for BIP30 duplicate tx (MarcoFalke) 77770d9 test: Properly serialize BIP34 coinbase height (MarcoFalke) Pull request description: This adds a test for BIP30 to check that duplicate txs can exist in the blockchain given the first one was completely spent when the second one is added. (Requested by ajtowns in bitcoin#16333 (comment)) We can not add a test that a later duplicate tx overwrites a previous one, because BIP30 is always enforced on regtest. If someone feels strongly about such a test, some Bitcoin Core code would have to be modified, which can be done in a follow up pull request. Also, add a commit to fix the BIP34 test failures reported in bitcoin#14633 (comment) ACKs for top commit: laanwj: Code review ACK fa8489a Tree-SHA512: c707d0bdc93937263876b603425b53322a2a9f9ec3f50716ae2fa9de8ddc644beb22b26c1bfde7f4aab102633e096b354ef380db919176bd2cb44a2828f884aa
jasonbcox
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Oct 13, 2020
Summary: In addition to the original commit, I had to add `from .script import OP_1`, which was skipped in our backport of PR11796 (D5750) Partial backport of [[bitcoin/bitcoin#16363 | PR16363]] Commit [[bitcoin/bitcoin@77770d9 | 77770d95e2838d7665fa8f621e9e83d79f9b3196]] Test Plan: `ninja && ninja check-functional` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7901
jasonbcox
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Oct 13, 2020
Summary: This adds a test for BIP30 to check that duplicate txs can exist in the blockchain given the first one was completely spent when the second one is added. Note that some lines modified in the Core PR were moved and deleted on our side (D5060, D6101) This concludes backport of Core [[bitcoin/bitcoin#16363 | PR16363]] Commit: [[bitcoin/bitcoin@fa8489a | fa8489a15511f61a372473927e73c34692bbec23]] Depends on D7901 Test Plan: `ninja && ./test/functional/test_runner.py feature_block` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7902
Munkybooty
pushed a commit
to Munkybooty/dash
that referenced
this pull request
Nov 25, 2021
fa8489a test: Add test for BIP30 duplicate tx (MarcoFalke) 77770d9 test: Properly serialize BIP34 coinbase height (MarcoFalke) Pull request description: This adds a test for BIP30 to check that duplicate txs can exist in the blockchain given the first one was completely spent when the second one is added. (Requested by ajtowns in bitcoin#16333 (comment)) We can not add a test that a later duplicate tx overwrites a previous one, because BIP30 is always enforced on regtest. If someone feels strongly about such a test, some Bitcoin Core code would have to be modified, which can be done in a follow up pull request. Also, add a commit to fix the BIP34 test failures reported in bitcoin#14633 (comment) ACKs for top commit: laanwj: Code review ACK fa8489a Tree-SHA512: c707d0bdc93937263876b603425b53322a2a9f9ec3f50716ae2fa9de8ddc644beb22b26c1bfde7f4aab102633e096b354ef380db919176bd2cb44a2828f884aa
Munkybooty
pushed a commit
to Munkybooty/dash
that referenced
this pull request
Nov 25, 2021
fa8489a test: Add test for BIP30 duplicate tx (MarcoFalke) 77770d9 test: Properly serialize BIP34 coinbase height (MarcoFalke) Pull request description: This adds a test for BIP30 to check that duplicate txs can exist in the blockchain given the first one was completely spent when the second one is added. (Requested by ajtowns in bitcoin#16333 (comment)) We can not add a test that a later duplicate tx overwrites a previous one, because BIP30 is always enforced on regtest. If someone feels strongly about such a test, some Bitcoin Core code would have to be modified, which can be done in a follow up pull request. Also, add a commit to fix the BIP34 test failures reported in bitcoin#14633 (comment) ACKs for top commit: laanwj: Code review ACK fa8489a Tree-SHA512: c707d0bdc93937263876b603425b53322a2a9f9ec3f50716ae2fa9de8ddc644beb22b26c1bfde7f4aab102633e096b354ef380db919176bd2cb44a2828f884aa
Munkybooty
pushed a commit
to Munkybooty/dash
that referenced
this pull request
Nov 30, 2021
fa8489a test: Add test for BIP30 duplicate tx (MarcoFalke) 77770d9 test: Properly serialize BIP34 coinbase height (MarcoFalke) Pull request description: This adds a test for BIP30 to check that duplicate txs can exist in the blockchain given the first one was completely spent when the second one is added. (Requested by ajtowns in bitcoin#16333 (comment)) We can not add a test that a later duplicate tx overwrites a previous one, because BIP30 is always enforced on regtest. If someone feels strongly about such a test, some Bitcoin Core code would have to be modified, which can be done in a follow up pull request. Also, add a commit to fix the BIP34 test failures reported in bitcoin#14633 (comment) ACKs for top commit: laanwj: Code review ACK fa8489a Tree-SHA512: c707d0bdc93937263876b603425b53322a2a9f9ec3f50716ae2fa9de8ddc644beb22b26c1bfde7f4aab102633e096b354ef380db919176bd2cb44a2828f884aa
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds a test for BIP30 to check that duplicate txs can exist in the blockchain given the first one was completely spent when the second one is added. (Requested by ajtowns in #16333 (comment))
We can not add a test that a later duplicate tx overwrites a previous one, because BIP30 is always enforced on regtest. If someone feels strongly about such a test, some Bitcoin Core code would have to be modified, which can be done in a follow up pull request.
Also, add a commit to fix the BIP34 test failures reported in #14633 (comment)