Re-write balanceTransaction using Node.evaluateTransactionBalance #3100

Anviking · 2022-01-27T20:30:24Z

Rewrite balanceTransaction using evaluateTransactionBalance
- Effects on goldens can be viewed in the diff of this PR
Enable prop_balanceTransactionBalanced
Polish
Even more polish
- Will look through from top to bottom before merging

Future work

Pay surplus as excess fee when we cannot afford a change-output. (Would like to do while comparing constructTx with balanceTx)
Ensure tx size limit is respected (Requires fixing ADP-1484 and this )

Comments

First part introducing goldens and the evaluateTransactionBalance function: #3150

Issue Number

ADP-1372

Anviking · 2022-02-17T17:21:02Z

bors try

iohk-bors · 2022-02-17T17:52:35Z

try

Build failed:

ci/hydra-build:required

Anviking · 2022-02-21T19:11:54Z

bors try

iohk-bors · 2022-02-21T20:15:10Z

try

Build succeeded:

3150: Prepare for rewriting balanceTransaction r=Anviking a=Anviking - [x] Add `evaluateTransactionBalance` to `TransactionLayer` - [x] Add `guardTxBalanced` to `balanceTransaction` - Tested indirectly by the fact that `guardTxBalanced` doesn't fail in our existing integration and unit tests. - [x] Add golden tests for `balanceTransaction` to ensure there are no regressions in #3100 ### Comments Preparation for #3100 ### Issue Number  ADP-1372 Co-authored-by: Johannes Lund <johannes.lund@iohk.io>

Anviking · 2022-03-01T13:13:10Z

bors try

If `SelectionStrategyOptimal` results in the tx max size being exceeded, try again with `SelectionStrategyMinimal`.

- Wrap very long lines. - Use 4 character indents. https://input-output-hk.github.io/adrestia/code/Coding-Standards#limit-line-length-to-80-characters

- Rename the property to make it clear that it checks more things than just whether the result is balanced. - make the feeExcess expectation better motivated. - polish a few other old comments

- Use 4-space indents everywhere. - Avoid vertical alignment. - Ensure that case expressions and pattern guards are indented one level to the left of their handlers.

These were introduced in a prior PR, but we might as well clean them up while we're adjusting this function. https://input-output-hk.github.io/adrestia/code/Coding-Standards#limit-line-length-to-80-characters

jonathanknowles · 2022-03-23T03:48:40Z

lib/core/src/Cardano/Wallet.hs

+        -- 'assignScriptRedeemers'. This should make the problem (1) unlikely,
+        -- but not necessarily impossible.
+        --
+        -- We should investigate and make sure to handle this better.


jonathanknowles · 2022-03-23T03:58:38Z

lib/core/src/Cardano/Wallet.hs

-                transform
+
+        lift $ traceWith tr $ MsgSelectionForBalancingStart
+            (CS.toExternalUTxOMap $ UTxOIndex.toMap internalUtxoAvailable)


I suspect this conversion can be avoided if we don't require ourselves to include a UTxO value in the MsgSelectionForBalancingStart value, since we only ever care about the size, which is the same before and after applying toExternalUTxOMap.

I'll push a commit that removes this conversion.

Fixed in 80f346c.

When recording details about the UTxO set in the log, we generally only care about the size of the UTxO set, rather than its composition. Therefore, we don't need to require ourselves to produce a value of type `UTxO` in order to create a log entry: we just need to obtain the size of the available UTxO set. Therefore, we can avoid converting from the coin selection's internal map of UTxO entries to values of the wallet's `UTxO` type, which requires traversing the entire UTxO set and converting every single entry. We can't avoid traversing the wallet's UTxO set at least once, in order to make a value of type `UTxOIndex`, but we can avoid doing it multiple times.

…lue`. This allows us to replace the pattern guard and the unsafe conversion with a single checked conversion.

jonathanknowles

This looks reasonable to me. I see that you've highlighted all areas for further work with TODO comments and links to tickets, which is great.

Thanks for making this PR!

👍🏻

jonathanknowles · 2022-03-23T04:30:14Z

bors r+

iohk-bors · 2022-03-23T06:27:18Z

Build succeeded:

Anviking self-assigned this Jan 27, 2022

Anviking force-pushed the anviking/ADP-1372/rewrite-balanceTx branch 3 times, most recently from 97084f0 to 80fa40c Compare February 3, 2022 19:17

Anviking changed the title ~~Re-write balanceTransaction using Node.evaluateTransactinoBalance~~ Re-write balanceTransaction using Node.evaluateTransactionBalance Feb 7, 2022

Anviking force-pushed the anviking/ADP-1372/rewrite-balanceTx branch 5 times, most recently from 9ca963a to f8ba97c Compare February 17, 2022 16:10

iohk-bors bot added a commit that referenced this pull request Feb 17, 2022

Try #3100:

562f807

Anviking force-pushed the anviking/ADP-1372/rewrite-balanceTx branch 2 times, most recently from bafe8cd to 416c120 Compare February 21, 2022 18:00

iohk-bors bot added a commit that referenced this pull request Feb 21, 2022

Try #3100:

900fc97

Anviking force-pushed the anviking/ADP-1372/rewrite-balanceTx branch 3 times, most recently from ca83c55 to 0c470e1 Compare February 22, 2022 17:07

Anviking mentioned this pull request Feb 22, 2022

Prepare for rewriting balanceTransaction #3150

Merged

3 tasks

Anviking force-pushed the anviking/ADP-1372/rewrite-balanceTx branch from 0c470e1 to 6f9a10c Compare February 22, 2022 21:26

Anviking force-pushed the anviking/ADP-1372/rewrite-balanceTx branch 3 times, most recently from 403fba6 to ae8a220 Compare March 1, 2022 13:12

iohk-bors bot added a commit that referenced this pull request Mar 1, 2022

Try #3100:

c823957

Anviking marked this pull request as ready for review March 1, 2022 13:15

Anviking and others added 17 commits March 23, 2022 02:39

Implement SelectionStrategyMinimal fallback in balanceTransaction

b4bdf3b

If `SelectionStrategyOptimal` results in the tx max size being exceeded, try again with `SelectionStrategyMinimal`.

fixup duplicate error

4b0a57b

Fixup: add Eq and Show

7af4d69

Fix a few formatting issues.

a003249

- Wrap very long lines. - Use 4 character indents. https://input-output-hk.github.io/adrestia/code/Coding-Standards#limit-line-length-to-80-characters

Reduce noise by moving deconstruction to helpers

3a29c09

Fixup: indentation

e69cbc4

prop_balanceTransaction{Balanced -> Valid} + polish

e43e32d

- Rename the property to make it clear that it checks more things than just whether the result is balanced. - make the feeExcess expectation better motivated. - polish a few other old comments

Add comment in genEncodingBoundaryLovelace

43431d1

Improve note about bad input-resolution: ADP-1544

57adb81

Update lib/core/src/Cardano/Wallet.hs

d7851c6

Add another note and link to ADP-1514

583e92c

Add prop_posAndNegFromCardanoValueRoundtrip test

57864d1

fixup: revert accidental increase to withMaxSuccess

4c5c4a5

clarify cbor ada bounds in comment

336513b

Minor fixes to indendation.

76a1a4d

- Use 4-space indents everywhere. - Avoid vertical alignment. - Ensure that case expressions and pattern guards are indented one level to the left of their handlers.

Fix some spelling errors.

ff80a02

Wrap long comments in balanceTransactionWithSelectionStrategy.

df3c9ba

These were introduced in a prior PR, but we might as well clean them up while we're adjusting this function. https://input-output-hk.github.io/adrestia/code/Coding-Standards#limit-line-length-to-80-characters

jonathanknowles force-pushed the anviking/ADP-1372/rewrite-balanceTx branch from 1138a59 to df3c9ba Compare March 23, 2022 03:22

jonathanknowles reviewed Mar 23, 2022

View reviewed changes

jonathanknowles added 2 commits March 23, 2022 03:59

Replace fromIntegral with intCastMaybe in `posAndNegFromCardanoVa…

0b57295

…lue`. This allows us to replace the pattern guard and the unsafe conversion with a single checked conversion.

jonathanknowles force-pushed the anviking/ADP-1372/rewrite-balanceTx branch from 60fcb27 to 0b57295 Compare March 23, 2022 04:27

jonathanknowles self-requested a review March 23, 2022 04:28

jonathanknowles approved these changes Mar 23, 2022

View reviewed changes

iohk-bors bot merged commit 3bf427b into master Mar 23, 2022

iohk-bors bot deleted the anviking/ADP-1372/rewrite-balanceTx branch March 23, 2022 06:27

WilliamKingNoel-Bot pushed a commit that referenced this pull request Mar 23, 2022

docs: Merge #3100

03025ab

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-write balanceTransaction using Node.evaluateTransactionBalance #3100

Re-write balanceTransaction using Node.evaluateTransactionBalance #3100

Anviking commented Jan 27, 2022 •

edited by jonathanknowles

Loading

Anviking commented Feb 17, 2022

iohk-bors bot commented Feb 17, 2022

Anviking commented Feb 21, 2022

iohk-bors bot commented Feb 21, 2022

Anviking commented Mar 1, 2022

jonathanknowles Mar 23, 2022

jonathanknowles Mar 23, 2022

jonathanknowles Mar 23, 2022

jonathanknowles left a comment

jonathanknowles commented Mar 23, 2022

iohk-bors bot commented Mar 23, 2022

Re-write balanceTransaction using Node.evaluateTransactionBalance #3100

Re-write balanceTransaction using Node.evaluateTransactionBalance #3100

Conversation

Anviking commented Jan 27, 2022 • edited by jonathanknowles Loading

Future work

Comments

Issue Number

Anviking commented Feb 17, 2022

iohk-bors bot commented Feb 17, 2022

try

Anviking commented Feb 21, 2022

iohk-bors bot commented Feb 21, 2022

try

Anviking commented Mar 1, 2022

jonathanknowles Mar 23, 2022

Choose a reason for hiding this comment

jonathanknowles Mar 23, 2022

Choose a reason for hiding this comment

jonathanknowles Mar 23, 2022

Choose a reason for hiding this comment

jonathanknowles left a comment

Choose a reason for hiding this comment

jonathanknowles commented Mar 23, 2022

iohk-bors bot commented Mar 23, 2022

Anviking commented Jan 27, 2022 •

edited by jonathanknowles

Loading