-
Notifications
You must be signed in to change notification settings - Fork 16
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
feat: more efficient bitcoin transaction packages #1181
base: main
Are you sure you want to change the base?
Conversation
creating a new one all the time
let above_limits = item_votes.count_ones() > self.max_votes_against | ||
|| self.total_vsize.saturating_add(item_vsize) > self.max_vsize; | ||
|
||
if above_limits { |
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.
nit: can we add to the fn docstring that the item may not be inserted?
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.
Updated in 1bd451a.
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.
mmm without having context the If no bag can fit the item then it is not added to any bag and is dropped
may seem contradicting the preceding Creates a new one if no bag exists that can fit the item
.
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.
🤦🏽
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.
Fixed in af48259
signer/src/bitcoin/utxo.rs
Outdated
}; | ||
|
||
let transactions = requests.construct_transactions().unwrap(); | ||
assert_eq!(transactions.len(), 25); | ||
assert_eq!(transactions.len(), 15); |
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.
IIUC this test was to check that we don't create a chain of more than MAX_TX_PER_BITCOIN_BLOCK
txs, if so we should probably change the votes in deposits and withdrawals to force creating singleton transactions (eg, adding a | 0b111
and incrementing both shifts by 3).
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.
Hmmm, I thought that it was to check that the vsize was capped by MEMPOOL_MAX_PACKAGE_SIZE
, but maybe it's both? Yeah, let's check both.
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.
Oh whoops, it's supposed to check that MAX_TX_PER_BITCOIN_BLOCK
is capped.
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.
Updated in 22eb346.
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.
nit: the comment we should generate 60 distinct transactions
is no longer true, given SbtcRequests
allow for up to 4 votes against it should generate only ~30 transactions.
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.
Fixed in 4641ddc
Description
Closes #1151 and closes #1047.
Changes
Testing Information
Added new unit tests for the new packager.
Checklist: