consensus: Simplify ConnectTrace #18685
Conversation
jnewbery
force-pushed
the
2020-04-connecttrace-simplify
branch
from
a9f84cc to
42f5e77
Compare
|
Thanks for the review @MarcoFalke . I've addressed your comments. |
|
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. |
|
Concept ACK |
robot-dreams
left a comment
robot-dreams
left a comment
There was a problem hiding this comment.
Concept ACK, thanks for doing this cleanup!
Unit tests passed locally
(Aside: What's the desired benefit of reviewers doing local testing, given the nice CI system? Is it to get more test runs on different system configurations?)
jnewbery
force-pushed
the
2020-04-connecttrace-simplify
branch
from
42f5e77 to
d956ac7
Compare
|
I've addressed the comment here #18685 (comment) and rebased on master (there wasn't a conflict but this was a few thousand commits behind master and it's a small change). |
robot-dreams
left a comment
robot-dreams
left a comment
There was a problem hiding this comment.
ACK d956ac72f11c0888c6d719d257aa050a37d12ff2
jnewbery
force-pushed
the
2020-04-connecttrace-simplify
branch
from
d956ac7 to
4faaaa1
Compare
jnewbery
changed the title
jnewbery
changed the title
ajtowns
left a comment
ajtowns
left a comment
There was a problem hiding this comment.
utACK 4faaaa1d5935e1296c624a8d9d414989ec2fede6
Clarify that the mempool is not guaranteed to be in a consistent state with the chain tip when BlockConnected and BlockDisconnected are fired. Remove outdated comment about BlockConnected providing a vector of transactions.
ConnectTrace is now only used to track blocks that were connected during an ActivateBestChainStep call. Simplify it to a typedef.
jnewbery
force-pushed
the
2020-04-connecttrace-simplify
branch
from
4faaaa1 to
16523a7
Compare
|
Thanks for the review, @ajtowns! I've addressed your comments and rebased on master (since this was a couple of thousand commits behind the current master). |
|
@ajtowns do you mind looking at this again? I've addressed your review comments. |
|
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
|
Closing due to lack of reviewer interest. |
|
Mark up for grabs? |
@Rspigler feel free to pick this up if you're interested. I'm happy to review, but this didn't get much reviewer interest last time round. |
7 participants
ConnectTrace is now only used to track blocks that were connected during
an ActivateBestChainStep call. Simplify it to a typedef and fix
comments.
Rather than trying to remove ConnectTrace, which would change the order that notifications are fired in the case of a re-org (#17562 (comment)), just simplify it and fix comments.