Expand functional zmq transaction tests #19507
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. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
This was referenced Jul 14, 2020
promag
reviewed
Jul 14, 2020
| # Mining the block with this tx should result in second notification | ||
| # after coinbase tx notification | ||
| self.nodes[0].generatetoaddress(1, ADDRESS_BCRT1_UNSPENDABLE) | ||
| hashtx.receive() |
There was a problem hiding this comment.
Could test coinbase:
- self.nodes[0].generatetoaddress(1, ADDRESS_BCRT1_UNSPENDABLE)
- hashtx.receive()
- txid = hashtx.receive()
- assert_equal(payment_txid, txid.hex())
+ hash = self.nodes[0].generatetoaddress(1, ADDRESS_BCRT1_UNSPENDABLE)[0]
+ coinbase_txid = self.nodes[0].getblock(hash)['tx'][0]
+ assert_equal(coinbase_txid, hashtx.receive().hex())
+ assert_equal(payment_txid, hashtx.receive().hex())|
code review ACK 7356292 |
|
Code review ACK 7356292.
@instagibbs will you update this branch? |
|
@promag not really no, heh. Removed the "yet" in OP. |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Aug 31, 2020
7356292 Have zmq reorg test cover mempool txns (Gregory Sanders) a0f4f9c Add zmq test for transaction pub during reorg (Gregory Sanders) 2399a06 Add test case for mempool->block zmq notification (Gregory Sanders) e70512a Make ordering of zmq consumption irrelevant to functional test (Gregory Sanders) Pull request description: Tests written to better define what messages are sent when. Also did a bit of refactoring to make sure the exact notification channel ordering doesn't matter. Confusions below aside, I believe having these more descriptive tests helps describe what behavior we expect from ZMQ notificaitons. Remaining confusion: 1) Notification patterns seem to vary wildly with the inclusion of mempool transactions being reorg'ed. See difference between "Add zmq test for transaction pub during reorg" and "Have zmq reorg test cover mempool txns" commits for specifics. 2) Why does a reorg'ed transaction get announced 3 times? From what I understand it can get announced once for disconnected block, once for mempool entry. What's the third? It occurs a 4th time when included in a block(not added in test) ACKs for top commit: laanwj: code review ACK 7356292 promag: Code review ACK 7356292. Tree-SHA512: 573662429523fd6a1af23dd907117320bc68cb51a93fba9483c9a2160bdce51fb590fcd97bcd2b2751d543d5c1148efa4e22e1c3901144f882b990ed2b450038
jasonbcox
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Sep 3, 2020
…ional test Summary: bitcoin/bitcoin@e70512a --- Depends on D7345 Partial backport of Core [[bitcoin/bitcoin#19507 | PR19507]] Test Plan: test_runner.py interface_zmq Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7346
jasonbcox
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Sep 3, 2020
Summary: bitcoin/bitcoin@2399a06 --- Depends on D7346 Partial backport of Core [[bitcoin/bitcoin#19507 | PR19507]] Test Plan: test_runner.py interface_zmq Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7347
deadalnix
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Sep 3, 2020
Summary: bitcoin/bitcoin@a0f4f9c --- note: disconnect_cb variable isn't used in this commit, it is and will be introduced in the next Depends on D7347 Partial backport of Core [[bitcoin/bitcoin#19507 | PR19507]] Test Plan: test_runner.py interface_zmq Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7348
deadalnix
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Sep 3, 2020
Summary: bitcoin/bitcoin@7356292 --- Depends on D7348 Concludes backport of Core [[bitcoin/bitcoin#19507 | PR19507]] Test Plan: test_runner.py interface_zmq Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7349
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.
Tests written to better define what messages are sent when. Also did a bit of refactoring to make sure the exact notification channel ordering doesn't matter.
Confusions below aside, I believe having these more descriptive tests helps describe what behavior we expect from ZMQ notificaitons.
Remaining confusion: