Skip to content

Commit 1368fbb

Browse files
fix(platform-wallet): a ProUpServTx always carries an output (#5105)
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent d22f8ab commit 1368fbb

1 file changed

Lines changed: 241 additions & 58 deletions

File tree

‎packages/rs-platform-wallet/src/masternode/update_service.rs‎

Lines changed: 241 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ use dashcore::blsful::{Bls12381G2Impl, SecretKey as BlsSecretKey, SignatureSchem
2626
use dashcore::hash_types::InputsHash;
2727
use dashcore::hashes::Hash;
2828
use dashcore::platform_node_id::PlatformNodeId;
29-
use dashcore::{Address as DashAddress, Network, Txid};
29+
use dashcore::{Address as DashAddress, Network, Transaction, Txid};
3030
use key_wallet::wallet::managed_wallet_info::transaction_builder::{
3131
BuilderError, TransactionBuilder, TransactionSigner,
3232
};
@@ -161,13 +161,14 @@ async fn fetch_operator_reward(
161161
operator_reward_from_registration(pro_tx_hash, &fetched.transaction)
162162
}
163163

164-
/// Read `operatorReward` out of a fetched registration transaction —
164+
/// Read `operatorReward` out of a fetched registration transaction,
165165
/// binding the response to the request first: DAPI's get-transaction reply
166166
/// is not authenticated, so the decoded transaction must hash to the
167-
/// SPV-authenticated proTxHash before its payload is trusted. Without this
168-
/// check a faulty or malicious endpoint could answer with an unrelated
169-
/// zero-reward ProRegTx and steer [`resolve_operator_payout_script`] into
170-
/// clearing a real operator payout.
167+
/// requested proTxHash before its payload is trusted. Only the operator
168+
/// reward is read from it; the service values come from the synced
169+
/// masternode list's entry for that proTxHash. Without the hash check a
170+
/// reply carrying an unrelated zero-reward ProRegTx would steer
171+
/// [`resolve_operator_payout_script`] into clearing a real operator payout.
171172
pub(crate) fn operator_reward_from_registration(
172173
pro_tx_hash: &[u8; 32],
173174
transaction: &dashcore::Transaction,
@@ -351,12 +352,88 @@ pub(crate) fn service_payload_fields(service: SocketAddr) -> (u128, u16) {
351352
(u128::from_le_bytes(octets), service.port())
352353
}
353354

355+
/// The funding builder for a ProUpServTx: the placeholder payload, the
356+
/// finalizer that commits it to the selected inputs and signs it, and one
357+
/// zero-value data output.
358+
///
359+
/// A ProUpServTx moves no value, so without that output a selection whose
360+
/// surplus is below the dust limit (all of it booked as fee, no change
361+
/// output) leaves the transaction with no outputs, which consensus refuses.
362+
/// The data output is the placeholder Dash Core's own `protx
363+
/// update_service` adds for the same reason, so the transaction always
364+
/// carries an output; change is still returned whenever the surplus clears
365+
/// the dust limit.
366+
pub(crate) fn update_service_builder(
367+
placeholder: ProviderUpdateServicePayload,
368+
operator_secret: Zeroizing<[u8; 32]>,
369+
) -> Result<TransactionBuilder, PlatformWalletError> {
370+
Ok(TransactionBuilder::new()
371+
.add_op_return(&[])
372+
.map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?
373+
.set_special_payload(TransactionPayload::ProviderUpdateServicePayloadType(
374+
placeholder,
375+
))
376+
.set_payload_finalizer(move |unsigned| {
377+
finalize_update_service_payload(unsigned, &operator_secret)
378+
}))
379+
}
380+
381+
/// The payload finalizer: write `inputs_hash` over the selected inputs and
382+
/// the operator-BLS `payload_sig` (basic scheme over `base_payload_hash()`,
383+
/// modern serialization: the exact convention `verify_message_digest`
384+
/// checks real mainnet signatures with).
385+
///
386+
/// Refuses a transaction without outputs before signing anything. key-wallet
387+
/// releases the build's reservation on any finalizer error, so the refusal
388+
/// leaves the selected inputs spendable.
389+
pub(crate) fn finalize_update_service_payload(
390+
unsigned: &Transaction,
391+
operator_secret: &[u8; 32],
392+
) -> Result<TransactionPayload, BuilderError> {
393+
if unsigned.output.is_empty() {
394+
return Err(BuilderError::InvalidData(
395+
"the assembled ProUpServTx has no outputs, which the network refuses".into(),
396+
));
397+
}
398+
let Some(TransactionPayload::ProviderUpdateServicePayloadType(placeholder)) =
399+
&unsigned.special_transaction_payload
400+
else {
401+
return Err(BuilderError::InvalidData(
402+
"the ProUpServTx placeholder payload is missing from the assembled transaction".into(),
403+
));
404+
};
405+
let mut finalized = placeholder.clone();
406+
finalized.inputs_hash = unsigned.hash_inputs();
407+
408+
let secret = Option::<BlsSecretKey<Bls12381G2Impl>>::from(
409+
BlsSecretKey::<Bls12381G2Impl>::from_be_bytes(operator_secret),
410+
)
411+
.ok_or_else(|| {
412+
BuilderError::SigningFailed("the operator key is not a valid BLS secret".into())
413+
})?;
414+
let signature = secret
415+
.sign(
416+
SignatureSchemes::Basic,
417+
finalized.base_payload_hash().as_byte_array(),
418+
)
419+
.map_err(|e| BuilderError::SigningFailed(format!("BLS payload signing failed: {e}")))?;
420+
let signature_bytes: [u8; 96] = signature
421+
.to_bytes_with_mode(dashcore::blsful::SerializationFormat::Modern)
422+
.as_slice()
423+
.try_into()
424+
.map_err(|_| {
425+
BuilderError::SigningFailed("BLS signature did not serialize to 96 bytes".into())
426+
})?;
427+
finalized.payload_sig = BLSSignature::from(signature_bytes);
428+
Ok(TransactionPayload::ProviderUpdateServicePayloadType(
429+
finalized,
430+
))
431+
}
432+
354433
/// Fund and finalize the ProUpServTx: input selection reserves the funding
355434
/// inputs, the payload finalizer writes `inputs_hash` and the operator-BLS
356-
/// `payload_sig` (basic scheme over `base_payload_hash()`, modern
357-
/// serialization — the exact convention `verify_message_digest` checks real
358-
/// mainnet signatures with), and only then are the inputs ECDSA-signed,
359-
/// since their sighashes cover the finished payload.
435+
/// `payload_sig`, and only then are the inputs ECDSA-signed, since their
436+
/// sighashes cover the finished payload.
360437
///
361438
/// Stops at the signed transaction; the caller broadcasts or abandons it.
362439
pub(crate) async fn build_sign_update_service<B, S>(
@@ -369,52 +446,7 @@ where
369446
B: TransactionBroadcaster + ?Sized,
370447
S: TransactionSigner + ?Sized + Sync,
371448
{
372-
let builder = TransactionBuilder::new()
373-
.set_special_payload(TransactionPayload::ProviderUpdateServicePayloadType(
374-
placeholder,
375-
))
376-
.set_payload_finalizer(move |unsigned| {
377-
let Some(TransactionPayload::ProviderUpdateServicePayloadType(placeholder)) =
378-
&unsigned.special_transaction_payload
379-
else {
380-
return Err(BuilderError::InvalidData(
381-
"the ProUpServTx placeholder payload is missing from the assembled \
382-
transaction"
383-
.into(),
384-
));
385-
};
386-
let mut finalized = placeholder.clone();
387-
finalized.inputs_hash = unsigned.hash_inputs();
388-
389-
let secret = Option::<BlsSecretKey<Bls12381G2Impl>>::from(
390-
BlsSecretKey::<Bls12381G2Impl>::from_be_bytes(&operator_secret),
391-
)
392-
.ok_or_else(|| {
393-
BuilderError::SigningFailed("the operator key is not a valid BLS secret".into())
394-
})?;
395-
let signature = secret
396-
.sign(
397-
SignatureSchemes::Basic,
398-
finalized.base_payload_hash().as_byte_array(),
399-
)
400-
.map_err(|e| {
401-
BuilderError::SigningFailed(format!("BLS payload signing failed: {e}"))
402-
})?;
403-
let signature_bytes: [u8; 96] = signature
404-
.to_bytes_with_mode(dashcore::blsful::SerializationFormat::Modern)
405-
.as_slice()
406-
.try_into()
407-
.map_err(|_| {
408-
BuilderError::SigningFailed(
409-
"BLS signature did not serialize to 96 bytes".into(),
410-
)
411-
})?;
412-
finalized.payload_sig = BLSSignature::from(signature_bytes);
413-
Ok(TransactionPayload::ProviderUpdateServicePayloadType(
414-
finalized,
415-
))
416-
});
417-
449+
let builder = update_service_builder(placeholder, operator_secret)?;
418450
core.finalize_transaction(builder, &SEND_FUNDING_SOURCES, 0, signer)
419451
.await
420452
}
@@ -430,9 +462,10 @@ mod tests {
430462
use super::super::list::test_support::{evonode, masternode};
431463
use super::*;
432464
use crate::broadcaster::BroadcastError;
433-
use crate::test_support::funded_wallet_manager;
465+
use crate::test_support::{
466+
funded_wallet_manager, funded_wallet_manager_with_outputs, WalletSigner,
467+
};
434468
use dashcore::blsful::{PublicKey as BlsPublicKey, Signature as BlsSignature};
435-
use dashcore::Transaction;
436469
use key_wallet::account::StandardAccountType;
437470
use std::sync::{Arc, Mutex};
438471

@@ -463,6 +496,40 @@ mod tests {
463496
}
464497
}
465498

499+
impl RecordingBroadcaster {
500+
fn sent_count(&self) -> usize {
501+
self.sent.lock().expect("broadcaster lock").len()
502+
}
503+
}
504+
505+
/// A BIP44-funded core wallet holding one UTXO per value in `outputs`,
506+
/// with a broadcaster that records what it is given.
507+
async fn core_with_outputs(
508+
outputs: &[u64],
509+
) -> (
510+
CoreWallet<RecordingBroadcaster>,
511+
Arc<RecordingBroadcaster>,
512+
WalletSigner,
513+
) {
514+
let (wallet_manager, wallet_id, generation, signer) =
515+
funded_wallet_manager_with_outputs(StandardAccountType::BIP44Account, outputs).await;
516+
let sdk = Arc::new(dash_sdk::SdkBuilder::new_mock().build().expect("mock sdk"));
517+
let broadcaster = Arc::new(RecordingBroadcaster::default());
518+
let core = CoreWallet::new(
519+
sdk,
520+
wallet_manager,
521+
wallet_id,
522+
broadcaster.clone(),
523+
generation,
524+
);
525+
(core, broadcaster, signer)
526+
}
527+
528+
fn regular_placeholder() -> ProviderUpdateServicePayload {
529+
let entry = operator_entry(0x44, false);
530+
prepare_update_service_placeholder(&entry, None, ScriptBuf::new()).expect("placeholder")
531+
}
532+
466533
/// The IPv4-mapped little-endian encoding, pinned against the known
467534
/// testnet ProUpServTx vector in dashcore's own payload tests
468535
/// (52.36.64.148:19999).
@@ -702,6 +769,10 @@ mod tests {
702769
let tx = &sent[0];
703770
assert_eq!(tx.txid(), txid);
704771
assert_eq!(tx.version, 3);
772+
assert!(
773+
!tx.output.is_empty(),
774+
"a ProUpServTx always carries an output"
775+
);
705776
assert!(
706777
tx.input.iter().all(|input| !input.script_sig.is_empty()),
707778
"every funding input is ECDSA-signed"
@@ -741,4 +812,116 @@ mod tests {
741812
.verify(&public_key, payload.base_payload_hash().as_byte_array())
742813
.expect("operator BLS signature verifies over base_payload_hash");
743814
}
815+
816+
/// A lone 600-duff UTXO covers the fee but leaves less than the dust
817+
/// limit over, so no change output is added. The zero-value data output
818+
/// still gives the transaction an output.
819+
#[tokio::test]
820+
async fn should_carry_an_output_when_the_only_utxo_leaves_no_change() {
821+
let (core, broadcaster, signer) = core_with_outputs(&[600]).await;
822+
823+
let prepared = build_sign_update_service(
824+
&core,
825+
regular_placeholder(),
826+
Zeroizing::new(OPERATOR_SECRET),
827+
&signer,
828+
)
829+
.await
830+
.expect("600 duffs pay the fee");
831+
832+
let tx = prepared.transaction();
833+
assert_eq!(tx.input.len(), 1);
834+
assert_eq!(tx.output.len(), 1, "one output even with no change");
835+
assert!(tx.output[0].script_pubkey.is_op_return());
836+
assert_eq!(tx.output[0].value, 0, "the data output moves no value");
837+
assert_eq!(prepared.fee(), 600, "the sub-dust surplus is the fee");
838+
assert_eq!(broadcaster.sent_count(), 0, "preparing must not broadcast");
839+
}
840+
841+
/// Branch-and-bound prefers the small UTXO, whose surplus is booked as
842+
/// fee; the transaction still carries an output.
843+
#[tokio::test]
844+
async fn should_carry_an_output_when_selection_prefers_the_small_utxo() {
845+
let (core, _broadcaster, signer) = core_with_outputs(&[1_000_000_000, 600]).await;
846+
847+
let prepared = build_sign_update_service(
848+
&core,
849+
regular_placeholder(),
850+
Zeroizing::new(OPERATOR_SECRET),
851+
&signer,
852+
)
853+
.await
854+
.expect("update service builds");
855+
856+
let tx = prepared.transaction();
857+
assert!(
858+
!tx.output.is_empty(),
859+
"a ProUpServTx always carries an output"
860+
);
861+
let Some(TransactionPayload::ProviderUpdateServicePayloadType(payload)) =
862+
&tx.special_transaction_payload
863+
else {
864+
panic!("the transaction must carry the ProUpServTx payload");
865+
};
866+
assert_eq!(payload.inputs_hash, tx.hash_inputs());
867+
}
868+
869+
/// A wallet that cannot cover the fee fails in coin selection, before
870+
/// anything is reserved, signed or sent.
871+
#[tokio::test]
872+
async fn should_fail_before_broadcast_when_the_fee_cannot_be_covered() {
873+
let (core, broadcaster, signer) = core_with_outputs(&[200]).await;
874+
875+
let err = build_sign_update_service(
876+
&core,
877+
regular_placeholder(),
878+
Zeroizing::new(OPERATOR_SECRET),
879+
&signer,
880+
)
881+
.await
882+
.expect_err("200 duffs cannot pay the fee");
883+
884+
assert!(
885+
matches!(err, PlatformWalletError::CorePooledInsufficientFunds { .. }),
886+
"an insufficient-funds error, got {err:?}"
887+
);
888+
assert_eq!(broadcaster.sent_count(), 0, "nothing is broadcast");
889+
}
890+
891+
/// Without the data output a lone 600-duff UTXO assembles into a
892+
/// transaction with no outputs. The finalizer refuses it before signing,
893+
/// and the refusal releases the reservation: the same UTXO funds the
894+
/// next build.
895+
#[tokio::test]
896+
async fn should_release_the_reservation_when_the_transaction_has_no_outputs() {
897+
let (core, broadcaster, signer) = core_with_outputs(&[600]).await;
898+
899+
let secret = Zeroizing::new(OPERATOR_SECRET);
900+
let without_output = TransactionBuilder::new()
901+
.set_special_payload(TransactionPayload::ProviderUpdateServicePayloadType(
902+
regular_placeholder(),
903+
))
904+
.set_payload_finalizer(move |unsigned| {
905+
finalize_update_service_payload(unsigned, &secret)
906+
});
907+
let err = core
908+
.finalize_transaction(without_output, &SEND_FUNDING_SOURCES, 0, &signer)
909+
.await
910+
.expect_err("a transaction with no outputs is refused");
911+
assert!(
912+
matches!(&err, PlatformWalletError::TransactionBuild(message) if message.contains("no outputs")),
913+
"the refusal names the missing outputs, got {err:?}"
914+
);
915+
assert_eq!(broadcaster.sent_count(), 0, "nothing is broadcast");
916+
917+
let rebuilt = build_sign_update_service(
918+
&core,
919+
regular_placeholder(),
920+
Zeroizing::new(OPERATOR_SECRET),
921+
&signer,
922+
)
923+
.await
924+
.expect("the released UTXO funds the next build");
925+
assert_eq!(rebuilt.transaction().input.len(), 1);
926+
}
744927
}

0 commit comments

Comments
 (0)