search

penumbra-zone / penumbra

Penumbra is a fully private proof-of-stake network and decentralized exchange for the Cosmos ecosystem.
https://penumbra.zone
Apache License 2.0
364 stars 288 forks source link

Completely remove all use of `unwrap` in `pd`; audit every use of `expect` #2920

Closed plaidfinch closed 10 months ago

plaidfinch commented 11 months ago

Bugs lurk where implicit panics can happen, see https://github.com/penumbra-zone/penumbra/issues/2919 for one recent example. We should never be using unwrap because it does not produce meaningful error messages or document the invariants that are being violated when the panic occurs.

We should audit every occurrence of unwrap and expect to ensure that we have fully documented the invariants that we believe hold and will never be violated. All unwraps should be converted to expects with good error messages. Any case where it is possible to return an Err variant instead, we should do that instead.

To aid in this task, I suggest that we add #![deny(clippy::unwrap_used)] at the crate level to anything in consensus (that is, pd and its dependencies). This will allow us to triage all uses of unwrap by following the clippy lints.

plaidfinch commented 11 months ago

As of now (do not treat this list as current):

$ rg --count "unwrap" | sort
crates/bin/pcli/build.rs:1
crates/bin/pcli/src/command/debug.rs:3
crates/bin/pcli/src/command/query/dex.rs:8
crates/bin/pcli/src/command/query/governance.rs:1
crates/bin/pcli/src/command/tx.rs:26
crates/bin/pcli/src/command/tx/replicate.rs:5
crates/bin/pcli/src/command/utils.rs:3
crates/bin/pcli/src/command/validator.rs:2
crates/bin/pcli/src/command/view/staked.rs:3
crates/bin/pcli/src/dex_utils/replicate/debug.rs:7
crates/bin/pcli/src/dex_utils/replicate/xyk.rs:6
crates/bin/pcli/src/main.rs:3
crates/bin/pcli/src/network.rs:1
crates/bin/pcli/src/opt.rs:2
crates/bin/pcli/tests/network_integration.rs:168
crates/bin/pcli/tests/proof.rs:19
crates/bin/pclientd/build.rs:1
crates/bin/pclientd/src/lib.rs:1
crates/bin/pclientd/tests/network_integration.rs:4
crates/bin/pd/build.rs:1
crates/bin/pd/src/events.rs:2
crates/bin/pd/src/info.rs:15
crates/bin/pd/src/info/specific.rs:8
crates/bin/pd/src/main.rs:5
crates/bin/pd/src/testnet.rs:6
crates/bin/pd/src/testnet/generate.rs:5
crates/bin/pd/src/testnet/join.rs:4
crates/core/app/src/action_handler/transaction.rs:9
crates/core/app/src/action_handler/transaction/stateful.rs:1
crates/core/app/src/action_handler/transaction/stateless.rs:1
crates/core/app/src/app/mod.rs:18
crates/core/app/src/governance/view.rs:3
crates/core/app/src/tests/swap_and_swap_claim.rs:16
crates/core/asset/src/asset.rs:9
crates/core/asset/src/asset/denom_metadata.rs:1
crates/core/asset/src/asset/registry.rs:6
crates/core/asset/src/balance.rs:2
crates/core/asset/src/balance/imbalance.rs:56
crates/core/asset/src/lib.rs:1
crates/core/asset/src/value.rs:24
crates/core/component/chain/src/component/app_hash.rs:4
crates/core/component/chain/src/component/view.rs:3
crates/core/component/chain/src/genesis/app_state.rs:9
crates/core/component/chain/src/lib.rs:3
crates/core/component/compact-block/src/compact_block.rs:1
crates/core/component/dao/src/component/view.rs:3
crates/core/component/dex/src/batch_swap_output_data.rs:28
crates/core/component/dex/src/component/action_handler/swap.rs:1
crates/core/component/dex/src/component/action_handler/swap_claim.rs:1
crates/core/component/dex/src/component/arb.rs:1
crates/core/component/dex/src/component/dex.rs:11
crates/core/component/dex/src/component/position_manager.rs:4
crates/core/component/dex/src/component/router/fill_route.rs:8
crates/core/component/dex/src/component/router/params.rs:7
crates/core/component/dex/src/component/router/path.rs:2
crates/core/component/dex/src/component/router/path_search.rs:1
crates/core/component/dex/src/component/router/tests.rs:300
crates/core/component/dex/src/component/swap_manager.rs:2
crates/core/component/dex/src/component/tests.rs:75
crates/core/component/dex/src/lp/nft.rs:4
crates/core/component/dex/src/lp/order.rs:12
crates/core/component/dex/src/lp/trading_function.rs:10
crates/core/component/dex/src/swap/plaintext.rs:3
crates/core/component/dex/src/swap/proof.rs:8
crates/core/component/dex/src/swap_claim/proof.rs:20
crates/core/component/dex/src/trading_pair.rs:2
crates/core/component/fee/src/fee.rs:3
crates/core/component/governance/src/delegator_vote/proof.rs:20
crates/core/component/governance/src/proposal_nft.rs:1
crates/core/component/governance/src/voting_receipt_token.rs:1
crates/core/component/ibc/src/component/channel.rs:4
crates/core/component/ibc/src/component/client.rs:24
crates/core/component/ibc/src/component/connection.rs:2
crates/core/component/ibc/src/component/msg_handler/channel_close_confirm.rs:1
crates/core/component/ibc/src/component/msg_handler/channel_close_init.rs:1
crates/core/component/ibc/src/component/msg_handler/channel_open_ack.rs:1
crates/core/component/ibc/src/component/msg_handler/channel_open_confirm.rs:1
crates/core/component/ibc/src/component/msg_handler/channel_open_init.rs:1
crates/core/component/ibc/src/component/msg_handler/channel_open_try.rs:2
crates/core/component/ibc/src/component/msg_handler/connection_open_ack.rs:3
crates/core/component/ibc/src/component/msg_handler/connection_open_confirm.rs:2
crates/core/component/ibc/src/component/msg_handler/connection_open_init.rs:2
crates/core/component/ibc/src/component/msg_handler/connection_open_try.rs:4
crates/core/component/ibc/src/component/msg_handler/create_client.rs:2
crates/core/component/ibc/src/component/msg_handler/update_client.rs:1
crates/core/component/ibc/src/component/packet.rs:3
crates/core/component/ibc/src/component/transfer.rs:14
crates/core/component/sct/src/component/view.rs:3
crates/core/component/shielded-pool/src/component/action_handler/output.rs:1
crates/core/component/shielded-pool/src/component/action_handler/spend.rs:1
crates/core/component/shielded-pool/src/component/note_manager.rs:5
crates/core/component/shielded-pool/src/component/shielded_pool.rs:2
crates/core/component/shielded-pool/src/component/supply.rs:2
crates/core/component/shielded-pool/src/note.rs:3
crates/core/component/shielded-pool/src/nullifier_derivation.rs:8
crates/core/component/shielded-pool/src/output/plan.rs:3
crates/core/component/shielded-pool/src/output/proof.rs:5
crates/core/component/shielded-pool/src/spend/proof.rs:35
crates/core/component/stake/src/action_handler/validator_definition.rs:4
crates/core/component/stake/src/component.rs:25
crates/core/component/stake/src/delegation_token.rs:1
crates/core/component/stake/src/penalty.rs:5
crates/core/component/stake/src/rate.rs:3
crates/core/component/stake/src/unbonding_token.rs:1
crates/core/component/stake/src/undelegate_claim/proof.rs:3
crates/core/component/stake/src/uptime.rs:4
crates/core/keys/src/keys/fvk.rs:2
crates/core/keys/src/keys/fvk/r1cs.rs:2
crates/core/keys/src/keys/ivk.rs:1
crates/core/keys/src/keys/seed_phrase.rs:2
crates/core/num/src/amount.rs:5
crates/core/num/src/fixpoint.rs:16
crates/core/transaction/src/action.rs:1
crates/core/transaction/src/auth_hash.rs:1
crates/core/transaction/src/effect_hash.rs:15
crates/core/transaction/src/memo.rs:2
crates/core/transaction/src/plan/action.rs:1
crates/core/transaction/src/plan/build.rs:4
crates/core/transaction/src/view/transaction_perspective.rs:5
crates/core/transaction/src/vote.rs:1
crates/crypto/decaf377-fmd/benches/fmd.rs:2
crates/crypto/decaf377-fmd/tests/fmd.rs:3
crates/crypto/decaf377-ka/tests/proptests.rs:6
crates/crypto/eddy/src/decryption_table.rs:1
crates/crypto/eddy/src/value.rs:4
crates/crypto/proof-params/benches/delegator_vote.rs:5
crates/crypto/proof-params/benches/nullifier_derivation.rs:2
crates/crypto/proof-params/benches/spend.rs:2
crates/crypto/proof-params/benches/swap.rs:2
crates/crypto/proof-params/benches/swap_claim.rs:4
crates/crypto/proof-params/build.rs:3
crates/crypto/proof-params/src/traits.rs:4
crates/crypto/proof-setup/src/log.rs:1
crates/crypto/proof-setup/src/phase2.rs:6
crates/crypto/tct/src/block.rs:1
crates/crypto/tct/src/commitment.rs:7
crates/crypto/tct/src/epoch.rs:3
crates/crypto/tct/src/internal/complete/node.rs:1
crates/crypto/tct/src/internal/frontier/node.rs:5
crates/crypto/tct/src/internal/frontier/tier.rs:2
crates/crypto/tct/src/internal/frontier/top.rs:5
crates/crypto/tct/src/internal/hash.rs:2
crates/crypto/tct/src/storage/deserialize.rs:1
crates/crypto/tct/src/structure.rs:3
crates/crypto/tct/src/tree.rs:13
crates/crypto/tct/src/validate.rs:2
crates/custody/build.rs:1
crates/custody/src/soft_kms/config.rs:3
crates/misc/measure/build.rs:1
crates/misc/measure/src/main.rs:6
crates/misc/tct-visualize/src/bin/tct-live-edit.rs:4
crates/misc/tct-visualize/src/bin/tct-visualize.rs:15
crates/misc/tct-visualize/src/live/control.rs:10
crates/misc/tct-visualize/src/live/view.rs:8
crates/misc/tct-visualize/src/live/view/earliest.rs:1
crates/misc/tct-visualize/src/render/dot.rs:2
crates/narsil/narsil/build.rs:1
crates/narsil/narsil/src/bin/narsild.rs:2
crates/narsil/narsil/src/ledger/app.rs:2
crates/narsil/narsil/src/ledger/info.rs:1
crates/proto/src/gen/penumbra.client.v1alpha1.rs:3
crates/proto/src/gen/penumbra.client.v1alpha1.serde.rs:97
crates/proto/src/gen/penumbra.core.chain.v1alpha1.serde.rs:34
crates/proto/src/gen/penumbra.core.crypto.v1alpha1.serde.rs:53
crates/proto/src/gen/penumbra.core.dex.v1alpha1.serde.rs:21
crates/proto/src/gen/penumbra.core.governance.v1alpha1.serde.rs:23
crates/proto/src/gen/penumbra.core.ibc.v1alpha1.serde.rs:15
crates/proto/src/gen/penumbra.core.stake.v1alpha1.serde.rs:36
crates/proto/src/gen/penumbra.core.transaction.v1alpha1.serde.rs:40
crates/proto/src/gen/penumbra.custody.v1alpha1.rs:1
crates/proto/src/gen/penumbra.custody.v1alpha1.serde.rs:3
crates/proto/src/gen/penumbra.narsil.ledger.v1alpha1.rs:1
crates/proto/src/gen/penumbra.narsil.ledger.v1alpha1.serde.rs:40
crates/proto/src/gen/penumbra.view.v1alpha1.rs:2
crates/proto/src/gen/penumbra.view.v1alpha1.serde.rs:33
crates/proto/src/serializers/bech32str.rs:1
crates/storage/src/delta.rs:15
crates/storage/src/future.rs:10
crates/storage/src/snapshot.rs:3
crates/storage/src/snapshot_cache.rs:7
crates/storage/src/storage.rs:3
crates/test/tct-property-test/tests/serialize.rs:3
crates/test/tct-property-test/tests/witness.rs:17
crates/util/tendermint-proxy/src/tendermint_proxy.rs:10
crates/view/build.rs:1
crates/view/src/service.rs:6
crates/view/src/storage.rs:12
crates/view/src/storage/sct.rs:14
crates/view/src/sync.rs:4
crates/view/src/worker.rs:5
crates/wallet/src/plan.rs:1
crates/wasm/src/lib.rs:19
crates/wasm/src/tx.rs:21
crates/wasm/src/view_server.rs:50
zbuc commented 10 months ago

Removing the existing use of unwrap is fairly straightforward but auditing the use of expect afterwards requires a lot of specific context and reasoning to determine those invariants, as well as thinking through scenarios where they might not hold due to behavior of chain participants.

Here are some places that had expect statements that concerned me:

(will update as I go)

zbuc commented 10 months ago

Closing this as complete, every consensus-critical and user-facing binary crate has been modified to have the clippy lint forbidding unwrap usage.

Auditing the usage of expect was split into a separate task, #3050