-
Notifications
You must be signed in to change notification settings - Fork 213
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
Address slow fee estimation on Byron wallets #2128
Conversation
@@ -266,7 +267,7 @@ spec = do | |||
Default | |||
payloadMigrate >>= flip verify | |||
[ expectResponseCode @IO HTTP.status202 | |||
, expectField id ((`shouldBe` 17). length) | |||
, expectField id ((`shouldBe` 20). length) |
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.
The fee estimation for Byron witnesses is slightly less accurate than before with this change, so fees are evaluated slightly in excess. We could spend more time tweaking numbers but I think it's good enough already even though it means that transactions costs a little bit more.
@@ -207,7 +208,7 @@ spec = do | |||
testAddressCycling ctx 3 | |||
testAddressCycling ctx 10 | |||
|
|||
it "BYRON_MIGRATE_01 - \ | |||
Hspec.it "BYRON_MIGRATE_01 - \ |
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.
we want to revert this to our it
, right?
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.
Using regular Hspec.it
makes sense here I think, because those tests use specific mnemonic wallets, so in case when those tests fail the subsequent run will result in 409
- wallet already exists anyway...
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.
Using regular Hspec.it makes sense here I think, because those tests use specific mnemonic wallets
Indeed. We don't want to retry these tests because they use a hard-coded mnemonic. So I changed it back to the standard hspec "it" to make debugging easier.
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.
What about prefixing the retrying version it
with sh
to clarify its function? (sorry, couldn't resist 😁)
@@ -167,7 +168,7 @@ spec = do | |||
testAddressCycling 3 | |||
testAddressCycling 10 | |||
|
|||
it "SHELLEY_MIGRATE_01_big_wallet - \ | |||
Hspec.it "SHELLEY_MIGRATE_01_big_wallet - \ |
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.
the same here
@@ -273,7 +274,7 @@ spec = do | |||
, expectErrorMessage (errMsg403NothingToMigrate srcId) | |||
] | |||
|
|||
it "SHELLEY_MIGRATE_02 - \ | |||
Hspec.it "SHELLEY_MIGRATE_02 - \ |
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.
the same here
@@ -338,22 +348,44 @@ data SomeBenchmarkResults where | |||
instance Buildable SomeBenchmarkResults where | |||
build (SomeBenchmarkResults results) = build results | |||
|
|||
data BenchRndResults s = BenchRndResults | |||
data WalletOverview = WalletOverview |
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.
lgtm.
Manually tested on testnet and fee estimation is now instantaneous on random and icarus Byron wallets.
(Would be nice to see full benchmarks after changes 😅)
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.
Resigning from ledger bits (using their serialization infrastructure) for performance reason and "hardcoding" input correction numbers basing on what we know in each case is included seems to be very good move. Especially if during fee estimation we invoke this part 100 times
@piotr-iohk, you're welcome:
|
bors r+ |
2128: Address slow fee estimation on Byron wallets r=KtorZ a=KtorZ # Issue Number <!-- Put here a reference to the issue this PR relates to and which requirements it tackles --> #2126 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - fc1da51 📍 **extend post-restoration benchmark wallet overview with number of addresses and transactions** We had only the UTxO, which gave little clarity about how many addresses and transactions did that correspond to. In practice, they're of the same order of magnitude, but it's good to have the actual number. - a234e49 📍 **do not construct dummy bootstrap witnesses but apply a correction on shelley witnesses afterwards** This is slightly simpler, but it also prevent some nasty things happening with the bootstrap witnesses: the 'Ord' instances used to compare and sort bootstrap witnesses require to serialize them fully, which involve two hashes of bytestrings; when done for 50 inputs, and 100 times in a row in multiple selection this little thing start to matter quite a lot. - fc9d4ff 📍 **adjust byron migration test assertion due to recent change** The fee estimation is now slightly faster, but the approximation is slightly worse, so we end up paying a little more for byron witnesses. # Comments <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: KtorZ <matthias.benkort@gmail.com>
…esses and transactions We had only the UTxO, which gave little clarity about how many addresses and transactions did that correspond to. In practice, they're of the same order of magnitude, but it's good to have the actual number.
…shelley witnesses afterwards This is slightly simpler, but it also prevent some nasty things happening with the bootstrap witnesses: the 'Ord' instances used to compare and sort bootstrap witnesses require to serialize them fully, which involve two hashes of bytestrings; when done for 50 inputs, and 100 times in a row in multiple selection this little thing start to matter quite a lot.
The fee estimation is now slightly faster, but the approximation is slightly worse, so we end up paying a little more for byron witnesses.
fc9d4ff
to
3e628f0
Compare
Canceled. |
bors r+ |
2121: Fix wallet address pool gap limited to Word8 r=KtorZ a=hasufell Although with Persistent Word32 will change the underlying type from SqlInt32 to SqlInt64, it doesn't matter for sqlite, since they all end up as INTEGER. https://www.sqlite.org/datatype3.html Issue #2120 2128: Address slow fee estimation on Byron wallets r=KtorZ a=KtorZ # Issue Number <!-- Put here a reference to the issue this PR relates to and which requirements it tackles --> #2126 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - fc1da51 📍 **extend post-restoration benchmark wallet overview with number of addresses and transactions** We had only the UTxO, which gave little clarity about how many addresses and transactions did that correspond to. In practice, they're of the same order of magnitude, but it's good to have the actual number. - a234e49 📍 **do not construct dummy bootstrap witnesses but apply a correction on shelley witnesses afterwards** This is slightly simpler, but it also prevent some nasty things happening with the bootstrap witnesses: the 'Ord' instances used to compare and sort bootstrap witnesses require to serialize them fully, which involve two hashes of bytestrings; when done for 50 inputs, and 100 times in a row in multiple selection this little thing start to matter quite a lot. - fc9d4ff 📍 **adjust byron migration test assertion due to recent change** The fee estimation is now slightly faster, but the approximation is slightly worse, so we end up paying a little more for byron witnesses. # Comments <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: Julian Ospald <julian.ospald@iohk.io> Co-authored-by: Julian Ospald <hasufell@posteo.de> Co-authored-by: KtorZ <matthias.benkort@gmail.com>
Build failed (retrying...): |
Build succeeded: |
Issue Number
#2126
Overview
fc1da51
📍 extend post-restoration benchmark wallet overview with number of addresses and transactions
We had only the UTxO, which gave little clarity about how many addresses and transactions did that correspond to. In practice, they're of the same order of magnitude, but it's good to have the actual number.
a234e49
📍 do not construct dummy bootstrap witnesses but apply a correction on shelley witnesses afterwards
This is slightly simpler, but it also prevent some nasty things happening with the bootstrap witnesses: the 'Ord' instances used to compare and sort bootstrap witnesses require to serialize them fully, which involve two hashes of bytestrings; when done for 50 inputs, and 100 times in a row in multiple selection this little thing start to matter quite a lot.
fc9d4ff
📍 adjust byron migration test assertion due to recent change
The fee estimation is now slightly faster, but the approximation is slightly worse, so we end up paying a little more for byron witnesses.
Comments