feat(tx): Add shielded outputs for amount and token privacy#1603
feat(tx): Add shielded outputs for amount and token privacy#1603
Conversation
c497c48 to
ecc157e
Compare
hathor/indexes/utxo_index.py
Outdated
| # remove all inputs | ||
| for tx_input in tx.inputs: | ||
| spent_tx = tx.get_spent_tx(tx_input) | ||
| # CONS-018: skip shielded outputs — they are never in the UTXO index |
There was a problem hiding this comment.
This change is wrong. It should actually include the utxo to the index regardless of the output type. But it might need some change when querying and filtering by value/token.
hathor/indexes/utxo_index.py
Outdated
| for tx_input in tx.inputs: | ||
| spent_tx = tx.get_spent_tx(tx_input) | ||
| # CONS-018: skip shielded outputs — they are never in the UTXO index | ||
| if spent_tx.is_shielded_output(tx_input.index): |
There was a problem hiding this comment.
Same here. The index should work with shielded outputs.
| assert len(vertex2.outputs) > txin.index, 'invalid output index' | ||
| # CONS-024: return None for shielded output indices instead of crashing | ||
| if txin.index >= len(vertex2.outputs): | ||
| return None |
There was a problem hiding this comment.
Why not return the shielded output?
| nanocontracts=False, | ||
| fee_tokens=False, | ||
| opcodes_version=OpcodesVersion.V1, | ||
| shielded_transactions=True, |
| class VertexHeaderId(Enum): | ||
| NANO_HEADER = b'\x10' | ||
| FEE_HEADER = b'\x11' | ||
| SHIELDED_OUTPUTS_HEADER = b'\x12' |
There was a problem hiding this comment.
Should headers be forced to be ordered according to the header id? This might prevent tx maleability.
| if txin.index < len(spent_tx.outputs): | ||
| output_script = spent_tx.outputs[txin.index].script | ||
| elif spent_tx.shielded_outputs: | ||
| shielded_idx = txin.index - len(spent_tx.outputs) | ||
| if shielded_idx >= len(spent_tx.shielded_outputs): | ||
| raise InvalidScriptError(f'input index {txin.index} out of range') | ||
| output_script = spent_tx.shielded_outputs[shielded_idx].script |
There was a problem hiding this comment.
Refactor to spent_tx.get_output(n) (or similar if you know a better name). Actually, maybe tx.outputs should be refactores to tx._outputs but that's a big task for this PR.
There was a problem hiding this comment.
Why not use resolve_spent_output()?
hathor/transaction/scripts/opcode.py
Outdated
| contract_value = spent_tx.outputs[txin.index].value | ||
| # CONS-020: use resolve_spent_output for shielded-aware lookup | ||
| resolved = spent_tx.resolve_spent_output(txin.index) | ||
| if not hasattr(resolved, 'value'): |
There was a problem hiding this comment.
Why not use output.mode? We just have to add it to the TxOutput as Transparent mode.
|
|
||
|
|
||
| class TrivialCommitmentError(TxValidationError): | ||
| """Rule 4: All transparent inputs require >= 2 shielded outputs.""" |
There was a problem hiding this comment.
Should we allow 1:1? Such transactions wouldn't actually shield the value.
| MAX_SHIELDED_OUTPUT_SCRIPT_SIZE = 1024 # Match settings.MAX_OUTPUT_SCRIPT_SIZE (VULN-001) | ||
|
|
||
|
|
||
| class ShieldedOutputMode(IntEnum): |
| """Return sighash bytes for a shielded output. | ||
|
|
||
| Includes commitment + mode + token_data/asset_commitment + script. | ||
| Does NOT include proofs (range_proof, surjection_proof). |
| def has_shielded_outputs(self) -> bool: | ||
| """Returns true if this transaction has a shielded outputs header.""" | ||
| try: | ||
| self.get_shielded_outputs_header() | ||
| except ValueError: | ||
| return False | ||
| else: | ||
| return True | ||
|
|
||
| def get_shielded_outputs_header(self) -> ShieldedOutputsHeader: | ||
| """Return the ShieldedOutputsHeader or raise ValueError.""" | ||
| return self._get_header(ShieldedOutputsHeader) | ||
|
|
||
| @property | ||
| def shielded_outputs(self) -> list[ShieldedOutput]: | ||
| """Return the list of shielded outputs, or empty list if no header.""" | ||
| if self.has_shielded_outputs(): | ||
| return self.get_shielded_outputs_header().shielded_outputs | ||
| return [] | ||
|
|
There was a problem hiding this comment.
All these methods should belong to BaseTransaction.
hathor/transaction/transaction.py
Outdated
| if tx_input.index < len(spent_tx.outputs): | ||
| spent_output = spent_tx.outputs[tx_input.index] | ||
| elif spent_tx.shielded_outputs: | ||
| shielded_idx = tx_input.index - len(spent_tx.outputs) | ||
| if shielded_idx < len(spent_tx.shielded_outputs): | ||
| # Shielded input: skip for token info (amount is hidden) | ||
| continue | ||
| else: | ||
| # Out of bounds — will be caught by _verify_inputs | ||
| continue | ||
| else: | ||
| # No shielded outputs and index out of bounds — will be caught by _verify_inputs | ||
| continue |
There was a problem hiding this comment.
Why not use resolve_spent_output()?
| """Shielded verifications that don't need storage.""" | ||
| try: | ||
| self.verifiers.shielded_tx.verify_shielded_outputs(tx) | ||
| except Exception: |
| assert self._settings.ENABLE_NANO_CONTRACTS | ||
| # nothing to do | ||
|
|
||
| if isinstance(vertex, Transaction) and vertex.has_shielded_outputs(): |
There was a problem hiding this comment.
You don't have to check for the vertex type since all of them have shielded outputs.
| if params.features.fee_tokens: | ||
| allowed_headers.add(FeeHeader) | ||
| # CONS-006: Token creation txs must NOT allow shielded outputs, | ||
| # as it bypasses minting verification when combined with verify_sum skip. |
There was a problem hiding this comment.
Remove "as it bypasses minting verification when combined with verify_sum skip".
|
|
||
| # CONS-025: verify headers are in canonical order (ascending VertexHeaderId) | ||
| if len(vertex.headers) > 1: | ||
| ids = [self._get_header_order(h) for h in vertex.headers] |
There was a problem hiding this comment.
Why not simply use the header index?
| token_creation_tx=token_creation_tx_verifier, | ||
| nano_header=nano_header_verifier, | ||
| on_chain_blueprint=on_chain_blueprint_verifier, | ||
| shielded_tx=shielded_tx_verifier, |
There was a problem hiding this comment.
There's no need for a shielded transaction verifier. Split the verifiers according to the vertex type, instead.
| for tx_input in tx.inputs: | ||
| tx2 = self.manager.tx_storage.get_transaction(tx_input.tx_id) | ||
| # CONS-021: skip shielded outputs — hidden amounts | ||
| if tx2.is_shielded_output(tx_input.index): |
There was a problem hiding this comment.
Should we add an "at least one shielded output" marker? If yes, it should be added to the response.
| for tx_input in tx.inputs: | ||
| spent_tx = tx.get_spent_tx(tx_input) | ||
| # CONS-022: skip shielded outputs | ||
| if spent_tx.is_shielded_output(tx_input.index): |
There was a problem hiding this comment.
Same here. Should we add a marker to the response?
| features = Features.from_vertex( | ||
| settings=self._settings, | ||
| feature_service=self.manager.feature_service, | ||
| vertex=best_block, | ||
| ) | ||
| params = VerificationParams.default_for_mempool(best_block=best_block, features=features) |
There was a problem hiding this comment.
Why this change? Doesn't VerificationParams already do it if features is not provided?
hathor/wallet/base_wallet.py
Outdated
| new_input = None | ||
| output_tx = tx_storage.get_transaction(_input.tx_id) | ||
| # CONS-023: skip shielded outputs | ||
| if output_tx.is_shielded_output(_input.index): |
There was a problem hiding this comment.
If the wallet is sending an input, it should be able to rewind the value and token id. So there would be no need to skip these.
hathor/wallet/base_wallet.py
Outdated
| assert tx.storage is not None | ||
| output_tx = tx.storage.get_transaction(_input.tx_id) | ||
| # CONS-023: skip shielded outputs | ||
| if output_tx.is_shielded_output(_input.index): |
| expected_tag = derive_tag(token_id) | ||
| expected_commitment = create_asset_commitment(expected_tag, asset_bf) |
There was a problem hiding this comment.
Shouldn't it be done by derive_tag() itself?
| # Get wallet private key for this address | ||
| private_key = self.get_private_key(script_type_out.address) | ||
| privkey_bytes, _ = extract_key_bytes(private_key) | ||
|
|
||
| # ECDH shared secret and deterministic nonce | ||
| shared_secret = derive_ecdh_shared_secret(privkey_bytes, output.ephemeral_pubkey) | ||
| nonce = derive_rewind_nonce(shared_secret) | ||
|
|
||
| # Determine generator for range proof rewind | ||
| if isinstance(output, AmountShieldedOutput): | ||
| token_index = output.token_data & 0x7F | ||
| token_uid = tx.get_token_uid(token_index) | ||
| # Normalize to 32 bytes | ||
| if len(token_uid) < 32: | ||
| token_uid = token_uid.ljust(32, b'\x00') | ||
| generator = derive_asset_tag(token_uid) | ||
| elif isinstance(output, FullShieldedOutput): | ||
| generator = output.asset_commitment | ||
| else: | ||
| continue |
There was a problem hiding this comment.
Refactor this and add a method in the shielded output that receives the private key and do it all.
| pub type TokenUid = [u8; 32]; | ||
|
|
||
| /// Zero token UID representing HTR. | ||
| pub const HTR_TOKEN_UID: TokenUid = [0u8; 32]; |
There was a problem hiding this comment.
Can we keep it with one byte?
b2e8bc2 to
c4c24e5
Compare
|
| Branch | feat/ct-amount-token-privacy |
| Testbed | ubuntu-22.04 |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result minutes (m) (Result Δ%) | Lower Boundary minutes (m) (Limit %) | Upper Boundary minutes (m) (Limit %) |
|---|---|---|---|---|
| sync-v2 (up to 20000 blocks) | 📈 view plot 🚷 view threshold | 1.64 m(-4.23%)Baseline: 1.71 m | 1.54 m (93.98%) | 2.05 m (79.81%) |
c4c24e5 to
bbb35b5
Compare
bbb35b5 to
2fb3a04
Compare
Motivation
What was the motivation for the changes in this PR?
Acceptance Criteria
Checklist
master, confirm this code is production-ready and can be included in future releases as soon as it gets merged