-
Notifications
You must be signed in to change notification settings - Fork 36.2k
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
test: fix misleading fee unit in mempool_limit.py #22972
test: fix misleading fee unit in mempool_limit.py #22972
Conversation
The helper `send_large_txs` in its current interface has a fee_rate parameter, implying that it would create a transaction with exactly that rate. Unfortunately, this fee rate is only passed to MiniWallet's `create_self_transfer` method, which can't know that we append several tx outputs after, increasing the tx's vsize and decreasing it's fee rate accordingly. In our case, the fee rate is off by several orders of magnitude, as the tx's vsize changes changes from 96 to 67552 vbytes (>700x), i.e. the value passed to this function is neither really a fee rate nor an absolute fee, but something in-between, which is very confusing. Clarify the interface by passing an absolute fee that is deducted in the end (and verified, via testmempoolaccept) and also describe how we come up with the value passed.
Concept ACK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK
the value passed to this function is neither really a fee rate nor an absolute fee
Definitely seems optimal to exclusively use absolute fee and fee rate rather than having some hard to define third variable
for _ in range(tx_batch_size): | ||
tx = miniwallet.create_self_transfer(from_node=node, fee_rate=fee_rate)['tx'] | ||
tx = miniwallet.create_self_transfer(from_node=node, fee_rate=0, mempool_valid=False)['tx'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this No, fee_rate
be replaced by fee
here too?create_self_transfer
has a fee_rate argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 2600db6.
This change clarifies the problem with the fee rates in the test.
The PR is a follow-up to #22543. The helper
send_large_txs
in its current interface has a fee_rate parameter, implying that it would create a transaction with exactly that rate. Unfortunately, this fee rate is only passed to MiniWallet'screate_self_transfer
method, which can't know that we append several tx outputs after, increasing the tx's vsize and decreasing it's fee rate accordingly.In our case, the fee rate is off by several orders of magnitude, as the tx's vsize changes changes from 96 to 67552 vbytes (>700x), i.e. the value passed to this function is neither really a fee rate nor an absolute fee, but something in-between, which is very confusing. It was suggested to simply in-line this helper as it's currently only used in this single test (#22543 (comment), #22543 (comment)), but I could imagine that this helper may also become useful for other tests and may be moved to a library (e.g. wallet.py) in the future.
Clarify the interface by passing an absolute fee that is deducted in the end (and also verified, via testmempoolaccept) and also describe how we come up with the value passed. On master, the comment says that the fee rate needs to increased "massively"; this word is also removed because the fee rate only needs to be higher for the test to succeed.