Skip to content

BIP119: Fix plotting of pending transactions for congestion control rates #1898

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MozirDmitriy
Copy link

Correct the assignment of variables when plotting "Confirmed but Pending Transactions" in the simulation. Now, the 1x rate line uses blocktimes_c1 and unspendable, and the 2x rate line uses blocktimes_c2 and unspendable2, ensuring the graph labels and data match correctly. This improves the accuracy and clarity of the simulation results visualization.

jonatack

This comment was marked as duplicate.

@jonatack jonatack added Proposed BIP modification Pending acceptance This BIP modification requires sign-off by the champion of the BIP being modified and removed Bug fix labels Jul 17, 2025
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MozirDmitriy thanks for your proposal. Would you please provide the images of the plot before and after this change, to compare with mine below, and explain how it is improved? Thanks!

For me, on master:

Figure_master

and this branch:

Figure_PR

Copy link
Contributor

@murchandamus murchandamus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image

As far as I can tell, the only differences between your two images are that the teal and red lines are slightly smoother and the latter is slightly shifted, but I could use a more thorough explanation what the bug in the original was and how your change improves the graph. Especially, I’d like to better understand why the variable was renamed.

p4, = par1.plot(blocktimes_c1, unspendable2, "c", label="Congestion Control Pending (2x Rate)")
p4, = par1.plot(blocktimes_c2, unspendable, "r", label="Congestion Control Pending (1x Rate)")
p4, = par1.plot(blocktimes_c1, unspendable, "c", label="Congestion Control Pending (1x Rate)")
p4b, = par1.plot(blocktimes_c2, unspendable2, "r", label="Congestion Control Pending (2x Rate)")
Copy link
Contributor

@murchandamus murchandamus Jul 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your comment explains what you did, but could you please further explain why the labels of the two lines should be swapped?

I’m also not sure whether the repetition of p4 in the original was a bug, but the change from p4 to p4b looks like a mistake, as p4b never appears again in this file.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your comment explains what you did, but could you please further explain why the labels of the two lines should be swapped?

I’m also not sure whether the repetition of p4 in the original was a bug, but the change from p4 to p4b looks like a mistake, as p4b never appears again in this file.

Now for 1x speed blocktimes_c1 and unspendable are used, and for 2x speed blocktimes_c2 and unspendable2 are used. This matches the simulation logic.
Labels now correctly reflect the data being displayed.
The second line uses a separate p4b variable, allowing both lines to be accessed if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pending acceptance This BIP modification requires sign-off by the champion of the BIP being modified Proposed BIP modification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants