Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Reject invalid scripts from our wallet #3230

Merged
merged 4 commits into from
Jul 22, 2021
Merged

Reject invalid scripts from our wallet #3230

merged 4 commits into from
Jul 22, 2021

Conversation

nfrisby
Copy link
Contributor

@nfrisby nfrisby commented Jun 28, 2021 •
edited
Loading

Fixes Issue #3205.

This PR introduces a flag to applyTx that lets us use different logic when the transaction was submitted by our local neighbor, the wallet.

@nfrisby nfrisby added the consensus issues related to ouroboros-consensus label Jun 28, 2021
@nfrisby nfrisby force-pushed the nfrisby/issue-3205-localtx-phase2-invalidity branch from f9450a9 to 8cbb107 Compare June 28, 2021 23:41
@nfrisby
Copy link
Contributor Author

nfrisby commented Jun 28, 2021

@EncodePanda @jasagredo What do you think about the new flag? Should we instead have introduced a new method? Is the naming reasonable? Should scriptsWereOK be its own class? Etc; please share your opinions -- the current decisions are just me following my initial instincts.

@nc6 nc6 mentioned this pull request Jun 29, 2021
Closed
@nfrisby nfrisby linked an issue Jun 29, 2021 that may be closed by this pull request
Don't punish local client for invalid scripts #3205
Closed
EncodePanda
EncodePanda previously approved these changes Jun 30, 2021
View reviewed changes
Copy link
Contributor

@EncodePanda EncodePanda left a comment

Choose a reason for hiding this comment

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

LGTM, minor suggestions, pls squash commits before merger or reword them

ouroboros-consensus-byronspec/src/Ouroboros/Consensus/ByronSpec/Ledger/Mempool.hs Outdated Show resolved Hide resolved
ouroboros-consensus/src/Ouroboros/Consensus/Ledger/SupportsMempool.hs Outdated Show resolved Hide resolved
@nfrisby nfrisby force-pushed the nfrisby/issue-3205-localtx-phase2-invalidity branch from 8cbb107 to c855f53 Compare July 4, 2021 16:34
@nfrisby
Copy link
Contributor Author

nfrisby commented Jul 4, 2021

Once PR #3241 merges, we can rebase this and it'll change far fewer files.

It currently includes that other PR's commit because this PR will change one of the Golden test files that that PR belatedly adds.

@nfrisby nfrisby requested a review from EncodePanda July 4, 2021 16:36
@nfrisby nfrisby force-pushed the nfrisby/issue-3205-localtx-phase2-invalidity branch 2 times, most recently from cb6aaac to 7cbcc4c Compare July 7, 2021 21:49
@nfrisby
Copy link
Contributor Author

nfrisby commented Jul 7, 2021 •
edited
Loading

The current commits are still reflective of my trailblazing; hence still Draft PR.

Also, this latest commit introduces some workarounds that will soon be properly provided by the ledger. The current tip is merely a proof of concept demonstrating that we can set type instance GenTx (ShelleyBlock era) = TxInBlock era.

Some Alonzo serialization golden tests fail, as expected. The Mary-to-Alonzo ThreadNet does pass; there are no Alonzo transactions in that, though, so it's not really proving much.

Edit: Also, when the ledger updates come through, we won't need to give Alonzo its own ApplyTxErr instance. So much of the complexity in the later commits currently on this PR will disappear.

@nfrisby nfrisby force-pushed the nfrisby/issue-3205-localtx-phase2-invalidity branch from 7cbcc4c to 03c525c Compare July 21, 2021 03:09
@nfrisby
Copy link
Contributor Author

nfrisby commented Jul 21, 2021 •
edited
Loading

@nc6 @EncodePanda would you review again please. Context/reasons for Draft:

  • This PR's cabal.project targets Redesign of Tx/TxInBlock cardano-ledger#2379 which I think will merge soon.

  • This PR currently includes the commits from Alexey's PR Flatten predicate failures #3268, which integrates some cardano-ledger-specs changes that Clarke's upstream PR is branched off of. It will also merge soon.

  • I have a couple stray diffs (eg the inotify-tools) that I still need to tidy up.

Edit: I've resolved all those things -- see my next comment here.

@nfrisby nfrisby force-pushed the nfrisby/issue-3205-localtx-phase2-invalidity branch from 03c525c to 4e548c3 Compare July 21, 2021 22:55
@nfrisby nfrisby marked this pull request as ready for review July 21, 2021 23:00
@nfrisby nfrisby requested review from jasagredo and removed request for coot, karknu and bolt12 July 21, 2021 23:00
@nfrisby
Copy link
Contributor Author

nfrisby commented Jul 21, 2021

@jasagredo @nc6 I now think this is ready for merge; please do a final review. Thanks!

@EncodePanda you're welcome to review it if you need a short mixup from your Genesis focus etc, but don't feel obligated.

@nfrisby nfrisby dismissed EncodePanda’s stale review July 22, 2021 12:39

Significant changes since that Approval

nc6
nc6 approved these changes Jul 22, 2021
View reviewed changes
Copy link
Contributor

@nc6 nc6 left a comment

Choose a reason for hiding this comment

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

One grammar fix, otherwise looks good.

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Eras.hs Show resolved Hide resolved
ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Eras.hs Outdated Show resolved Hide resolved
@nfrisby
consensus: change extendVRNew argument order
4c6bc9b
@nfrisby
consensus: add WhetherToIntevene to applyTx
415e25d 
We introduce this argument so that we can use slightly different rules when
validating transactions depending on their source. In particular, we can reject
an invalid script submitted by the local wallet, thereby preventing the local
wallet from being punished for it. This is not something we want to do for
transactions from peers. The new flag controls eg whether we intervene in this
way.
@nfrisby
consensus: make extractTxs return Txs not Validated Txs
23bac07 
The next commit will increase the cardano-ledger-specs. That update introduces
a newtype wrapper which precludes the current type of extractTxs. This function
is only used in tests, for inspecting block contents, so it's not problematic
to change its type in this way.
@nfrisby nfrisby mentioned this pull request Nov 30, 2023
Open
@nfrisby
consensus-shelley: new cardano-ledger-specs, IsValidating flag now in…
46cc6a6 
… Alonzo Tx

Instead of intervening whenever the local wallet submits a transaction with an
invalid script, we now require the local wallet tell us whether it thinks the
submitted scripts are valid. We only intervene if the wallet's claim is
incorrect. (At least at first, the wallet will likely only ever intend to
submit valid scripts -- so nothing is really new here.)

It also makes sense to do the same thing for txs submitted from peers: force
them to explicity state a claim about the scripts' validity. Except in that
case we don't intervene as above; we instead simply propagate the tx with a
corrected flag and -- not yet aa part of this PR -- disconnect from the buggy
peer.
@nfrisby nfrisby force-pushed the nfrisby/issue-3205-localtx-phase2-invalidity branch from 4e548c3 to 46cc6a6 Compare July 22, 2021 23:27
@nfrisby
Copy link
Contributor Author

nfrisby commented Jul 22, 2021

I addressed Clarke's comments and opened the IntersectMBO/ouroboros-consensus#589 follow-up. Rebased onto origin/master, which was automatic with no conflicts. Thanks, reviewers!

@nfrisby
Copy link
Contributor Author

nfrisby commented Jul 22, 2021

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 22, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 36cb0b9 into master Jul 22, 2021
@iohk-bors iohk-bors bot deleted the nfrisby/issue-3205-localtx-phase2-invalidity branch July 22, 2021 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nc6 nc6 nc6 approved these changes

@EncodePanda EncodePanda Awaiting requested review from EncodePanda

@jasagredo jasagredo Awaiting requested review from jasagredo

Assignees
No one assigned
Labels
consensus issues related to ouroboros-consensus
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Don't punish local client for invalid scripts
3 participants
@nfrisby @nc6 @EncodePanda