From c9476ce52ff2538d9672ae919d39b926c02ae11b Mon Sep 17 00:00:00 2001 From: JSKitty Date: Thu, 31 Oct 2024 21:54:15 +0000 Subject: [PATCH 1/8] add: Stake Splitting for improved staking efficiency --- chain_params.prod.json | 6 ++++-- chain_params.test.json | 6 ++++-- scripts/wallet.js | 26 +++++++++++++++++++++----- 3 files changed, 29 insertions(+), 9 deletions(-) diff --git a/chain_params.prod.json b/chain_params.prod.json index edd2249a0..74f36d0f0 100644 --- a/chain_params.prod.json +++ b/chain_params.prod.json @@ -32,7 +32,8 @@ "proposalFeeConfirmRequirement": 6, "maxPaymentCycles": 6, "maxPayment": 43200000000000, - "defaultColdStakingAddress": "SdgQDpS8jDRJDX8yK8m9KnTMarsE84zdsy" + "defaultColdStakingAddress": "SdgQDpS8jDRJDX8yK8m9KnTMarsE84zdsy", + "stakeSplitTarget": 50000000000 }, "testnet": { "name": "testnet", @@ -64,6 +65,7 @@ "proposalFeeConfirmRequirement": 3, "maxPaymentCycles": 20, "maxPayment": 144000000000, - "defaultColdStakingAddress": "WmNziUEPyhnUkiVdfsiNX93H6rSJnios44" + "defaultColdStakingAddress": "WmNziUEPyhnUkiVdfsiNX93H6rSJnios44", + "stakeSplitTarget": 50000000000 } } diff --git a/chain_params.test.json b/chain_params.test.json index ed5a06cb0..ba00454a5 100644 --- a/chain_params.test.json +++ b/chain_params.test.json @@ -24,7 +24,8 @@ "proposalFeeConfirmRequirement": 3, "maxPaymentCycles": 20, "maxPayment": 144000000000, - "defaultColdStakingAddress": "WmNziUEPyhnUkiVdfsiNX93H6rSJnios44" + "defaultColdStakingAddress": "WmNziUEPyhnUkiVdfsiNX93H6rSJnios44", + "stakeSplitTarget": 50000000000 }, "testnet": { "name": "testnet", @@ -54,6 +55,7 @@ "proposalFeeConfirmRequirement": 3, "maxPaymentCycles": 20, "maxPayment": 144000000000, - "defaultColdStakingAddress": "WmNziUEPyhnUkiVdfsiNX93H6rSJnios44" + "defaultColdStakingAddress": "WmNziUEPyhnUkiVdfsiNX93H6rSJnios44", + "stakeSplitTarget": 50000000000 } } diff --git a/scripts/wallet.js b/scripts/wallet.js index 1f5102325..79e678aef 100644 --- a/scripts/wallet.js +++ b/scripts/wallet.js @@ -1012,11 +1012,27 @@ export class Wallet { // Add primary output if (isDelegation) { if (!returnAddress) [returnAddress] = this.getNewAddress(1); - transactionBuilder.addColdStakeOutput({ - address: returnAddress, - addressColdStake: address, - value, - }); + // The per-output target for maximum staking efficiency + const nTarget = cChainParams.current.stakeSplitTarget; + // Keep track of the remainder, which is the 'unoptimal' output (or change/non-output, if too small) + let nRemainder = value; + // Generate optimal staking outputs + while (nRemainder > nTarget) { + transactionBuilder.addColdStakeOutput({ + address: returnAddress, + addressColdStake: address, + value: nTarget, + }); + nRemainder -= nTarget; + } + // If the remainder is larger than one coin, we create the 'unoptimal' output + if (nRemainder >= COIN) { + transactionBuilder.addColdStakeOutput({ + address: returnAddress, + addressColdStake: address, + value: nRemainder, + }); + } } else if (isProposal) { transactionBuilder.addProposalOutput({ hash: address, From d0964473a94c67b0db4cfb62aeb1b7aded95c87c Mon Sep 17 00:00:00 2001 From: JSKitty Date: Thu, 31 Oct 2024 21:55:34 +0000 Subject: [PATCH 2/8] fix: use TxBuilder's `valueOut` instead of the user's `value` input This was causing change to be computed incorrectly if the actual `valueOut` differed from the user's `value` due to automated differences (i.e: Stake Split remainders) --- scripts/wallet.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/wallet.js b/scripts/wallet.js index 79e678aef..4c8ca862a 100644 --- a/scripts/wallet.js +++ b/scripts/wallet.js @@ -1062,7 +1062,7 @@ export class Wallet { } const fee = transactionBuilder.getFee(); - const changeValue = transactionBuilder.valueIn - value - fee; + const changeValue = transactionBuilder.valueIn - transactionBuilder.valueOut - fee; if (changeValue < 0) { if (!subtractFeeFromAmt) { throw new Error('Not enough balance'); From aca0134fc5b509a030fa8f091b3a5724cce6e16c Mon Sep 17 00:00:00 2001 From: JSKitty Date: Thu, 31 Oct 2024 22:12:15 +0000 Subject: [PATCH 3/8] Prettier --- scripts/wallet.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts/wallet.js b/scripts/wallet.js index 4c8ca862a..2a9609780 100644 --- a/scripts/wallet.js +++ b/scripts/wallet.js @@ -1062,7 +1062,8 @@ export class Wallet { } const fee = transactionBuilder.getFee(); - const changeValue = transactionBuilder.valueIn - transactionBuilder.valueOut - fee; + const changeValue = + transactionBuilder.valueIn - transactionBuilder.valueOut - fee; if (changeValue < 0) { if (!subtractFeeFromAmt) { throw new Error('Not enough balance'); From 924bcac16523144eb7729e8478d5cdf9084264a6 Mon Sep 17 00:00:00 2001 From: JSKitty Date: Fri, 1 Nov 2024 09:39:47 +0000 Subject: [PATCH 4/8] refactor: add review suggestion --- scripts/wallet.js | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/scripts/wallet.js b/scripts/wallet.js index 2a9609780..a1fe07e24 100644 --- a/scripts/wallet.js +++ b/scripts/wallet.js @@ -1014,23 +1014,12 @@ export class Wallet { if (!returnAddress) [returnAddress] = this.getNewAddress(1); // The per-output target for maximum staking efficiency const nTarget = cChainParams.current.stakeSplitTarget; - // Keep track of the remainder, which is the 'unoptimal' output (or change/non-output, if too small) - let nRemainder = value; // Generate optimal staking outputs - while (nRemainder > nTarget) { + for (let i = 0; i < Math.floor(value / nTarget); i++) { transactionBuilder.addColdStakeOutput({ address: returnAddress, addressColdStake: address, - value: nTarget, - }); - nRemainder -= nTarget; - } - // If the remainder is larger than one coin, we create the 'unoptimal' output - if (nRemainder >= COIN) { - transactionBuilder.addColdStakeOutput({ - address: returnAddress, - addressColdStake: address, - value: nRemainder, + value: i == 0 ? nTarget + (value % nTarget) : nTarget, }); } } else if (isProposal) { From 8a69f52bc383e7861cbafc9ac040e0de5ba1edbb Mon Sep 17 00:00:00 2001 From: JSKitty Date: Fri, 8 Nov 2024 14:26:05 +0000 Subject: [PATCH 5/8] fix: eqeqeq linting error --- scripts/wallet.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/wallet.js b/scripts/wallet.js index ca1a08442..3e1afa02f 100644 --- a/scripts/wallet.js +++ b/scripts/wallet.js @@ -1018,7 +1018,7 @@ export class Wallet { transactionBuilder.addColdStakeOutput({ address: returnAddress, addressColdStake: address, - value: i == 0 ? nTarget + (value % nTarget) : nTarget, + value: i === 0 ? nTarget + (value % nTarget) : nTarget, }); } } else if (isProposal) { From ab3a1a0f84c431a744cf55f0d2669faa272f80c3 Mon Sep 17 00:00:00 2001 From: JSKitty Date: Fri, 8 Nov 2024 15:35:20 +0000 Subject: [PATCH 6/8] fix: avoid delegating dust --- scripts/wallet.js | 8 ++++++-- tests/unit/wallet/transactions.spec.js | 1 + 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/scripts/wallet.js b/scripts/wallet.js index 3e1afa02f..2178bf735 100644 --- a/scripts/wallet.js +++ b/scripts/wallet.js @@ -1014,11 +1014,15 @@ export class Wallet { // The per-output target for maximum staking efficiency const nTarget = cChainParams.current.stakeSplitTarget; // Generate optimal staking outputs - for (let i = 0; i < Math.floor(value / nTarget); i++) { + for (let i = 0; i <= Math.floor(value / nTarget); i++) { + const nOutAmount = i === 0 ? value % nTarget : nTarget; + // Skip 'dust' outputs (TODO: this should be `1 * COIN` as it is the consensus minimum, but tests are below this and will break) + // ... it's still better than no check, or `Stake 5 PIV` would create a 0-value remainder and fail. + if (nOutAmount < 0.05 * COIN) continue; transactionBuilder.addColdStakeOutput({ address: returnAddress, addressColdStake: address, - value: i === 0 ? nTarget + (value % nTarget) : nTarget, + value: nOutAmount, }); } } else if (isProposal) { diff --git a/tests/unit/wallet/transactions.spec.js b/tests/unit/wallet/transactions.spec.js index 338926cf6..1f75e8241 100644 --- a/tests/unit/wallet/transactions.spec.js +++ b/tests/unit/wallet/transactions.spec.js @@ -165,6 +165,7 @@ describe('Wallet transaction tests', () => { }); it('Creates a cold stake tx correctly', async () => { + // TODO: rewrite this test over >1 PIV to match PIVX Consensus, and to also test Stake Splitting const tx = wallet.createTransaction( 'SR3L4TFUKKGNsnv2Q4hWTuET2a4vHpm1b9', 0.05 * 10 ** 8, From 93d88244ee9bc229de16ffc288a961fad7c74992 Mon Sep 17 00:00:00 2001 From: JSKitty Date: Tue, 3 Dec 2024 15:50:46 +0000 Subject: [PATCH 7/8] tests: add Pre-Split + Dust Change testing --- scripts/wallet.js | 5 ++-- tests/unit/wallet/transactions.spec.js | 40 +++++++++++++++++--------- 2 files changed, 29 insertions(+), 16 deletions(-) diff --git a/scripts/wallet.js b/scripts/wallet.js index 59e789b2f..0600f3ba4 100644 --- a/scripts/wallet.js +++ b/scripts/wallet.js @@ -1169,9 +1169,8 @@ export class Wallet { // Generate optimal staking outputs for (let i = 0; i <= Math.floor(value / nTarget); i++) { const nOutAmount = i === 0 ? value % nTarget : nTarget; - // Skip 'dust' outputs (TODO: this should be `1 * COIN` as it is the consensus minimum, but tests are below this and will break) - // ... it's still better than no check, or `Stake 5 PIV` would create a 0-value remainder and fail. - if (nOutAmount < 0.05 * COIN) continue; + // Skip unstakeable outputs < 1 COIN + if (nOutAmount < COIN) continue; transactionBuilder.addColdStakeOutput({ address: returnAddress, addressColdStake: address, diff --git a/tests/unit/wallet/transactions.spec.js b/tests/unit/wallet/transactions.spec.js index e1ecc016e..2f80a5d66 100644 --- a/tests/unit/wallet/transactions.spec.js +++ b/tests/unit/wallet/transactions.spec.js @@ -14,6 +14,7 @@ import { import 'fake-indexeddb/auto'; import { TransactionBuilder } from '../../../scripts/transaction_builder.js'; +import { cChainParams } from '../../../scripts/chain_params.js'; vi.stubGlobal('localStorage', { length: 0 }); vi.mock('../../../scripts/global.js'); @@ -169,10 +170,10 @@ describe('Wallet transaction tests', () => { }); it('Creates a cold stake tx correctly', async () => { - // TODO: rewrite this test over >1 PIV to match PIVX Consensus, and to also test Stake Splitting + // Delegate 5250 PIV to test Stake Pre-Splitting const tx = wallet.createTransaction( 'SR3L4TFUKKGNsnv2Q4hWTuET2a4vHpm1b9', - 0.05 * 10 ** 8, + 5250 * 10 ** 8, { isDelegation: true } ); expect(tx.version).toBe(1); @@ -185,22 +186,33 @@ describe('Wallet transaction tests', () => { scriptSig: '76a914f49b25384b79685227be5418f779b98a6be4c73888ac', // Script sig must be the UTXO script since it's not signed }) ); - expect(tx.vout[1]).toStrictEqual( + // The 'after split' output + expect(tx.vout[0]).toStrictEqual( new CTxOut({ - script: '76a914f49b25384b79685227be5418f779b98a6be4c73888ac', - value: 4997470, + script: '76a97b63d114291a25b5b4d1802e0611e9bf724a1e57d9210e826714f49b25384b79685227be5418f779b98a6be4c7386888ac', + value: 25000000000, }) ); - expect(tx.vout[0]).toStrictEqual( + // The split outputs (depending on chainparam 'stakeSplitTarget') + for (const cOut of tx.vout.slice(1, tx.vout.length - 1)) { + expect(cOut).toStrictEqual( + new CTxOut({ + script: '76a97b63d114291a25b5b4d1802e0611e9bf724a1e57d9210e826714f49b25384b79685227be5418f779b98a6be4c7386888ac', + value: cChainParams.current.stakeSplitTarget, + }) + ); + } + // Undelegated change + expect(tx.vout[11]).toStrictEqual( new CTxOut({ - script: '76a97b63d114291a25b5b4d1802e0611e9bf724a1e57d9210e826714f49b25384b79685227be5418f779b98a6be4c7386888ac', - value: 5000000, + script: '76a914f49b25384b79685227be5418f779b98a6be4c73888ac', + value: 475009989980, }) ); await checkFees(wallet, tx, MIN_FEE_PER_BYTE); }); - it('creates a tx with max balance', async () => { + it('Creates a cold stake tx with max balance', async () => { const tx = wallet.createTransaction( 'SR3L4TFUKKGNsnv2Q4hWTuET2a4vHpm1b9', legacyMainnetInitialBalance(), @@ -217,12 +229,14 @@ describe('Wallet transaction tests', () => { scriptSig: '76a914f49b25384b79685227be5418f779b98a6be4c73888ac', // Script sig must be the UTXO script since it's not signed }) ); - expect(tx.vout).toHaveLength(1); + // Cold Stake split outputs + expect(tx.vout).toHaveLength(21); + // Undelegated dust change (<1 coin, too small to stake) const fees = await checkFees(wallet, tx, MIN_FEE_PER_BYTE); - expect(tx.vout[0]).toStrictEqual( + expect(tx.vout[20]).toStrictEqual( new CTxOut({ - script: '76a97b63d114291a25b5b4d1802e0611e9bf724a1e57d9210e826714f49b25384b79685227be5418f779b98a6be4c7386888ac', - value: legacyMainnetInitialBalance() - fees, + script: '76a914f49b25384b79685227be5418f779b98a6be4c73888ac', + value: 10000000 - fees, }) ); }); From f5ab7d0f2b1a086d568b31d94fff41dfa4ccc2cf Mon Sep 17 00:00:00 2001 From: JSKitty Date: Tue, 3 Dec 2024 16:50:19 +0000 Subject: [PATCH 8/8] Add review suggestion --- scripts/wallet.js | 17 ++++++++++++----- tests/unit/wallet/transactions.spec.js | 22 +++++++--------------- 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/scripts/wallet.js b/scripts/wallet.js index 0600f3ba4..561c57986 100644 --- a/scripts/wallet.js +++ b/scripts/wallet.js @@ -1167,15 +1167,22 @@ export class Wallet { // The per-output target for maximum staking efficiency const nTarget = cChainParams.current.stakeSplitTarget; // Generate optimal staking outputs - for (let i = 0; i <= Math.floor(value / nTarget); i++) { - const nOutAmount = i === 0 ? value % nTarget : nTarget; - // Skip unstakeable outputs < 1 COIN - if (nOutAmount < COIN) continue; + if (value < COIN) { + throw new Error('below consensus'); + } else if (value < nTarget) { transactionBuilder.addColdStakeOutput({ address: returnAddress, addressColdStake: address, - value: nOutAmount, + value, }); + } else { + for (let i = 0; i < Math.floor(value / nTarget); i++) { + transactionBuilder.addColdStakeOutput({ + address: returnAddress, + addressColdStake: address, + value: i === 0 ? nTarget + (value % nTarget) : nTarget, + }); + } } } else if (isProposal) { transactionBuilder.addProposalOutput({ diff --git a/tests/unit/wallet/transactions.spec.js b/tests/unit/wallet/transactions.spec.js index 2f80a5d66..ce618d240 100644 --- a/tests/unit/wallet/transactions.spec.js +++ b/tests/unit/wallet/transactions.spec.js @@ -190,7 +190,7 @@ describe('Wallet transaction tests', () => { expect(tx.vout[0]).toStrictEqual( new CTxOut({ script: '76a97b63d114291a25b5b4d1802e0611e9bf724a1e57d9210e826714f49b25384b79685227be5418f779b98a6be4c7386888ac', - value: 25000000000, + value: 75000000000, }) ); // The split outputs (depending on chainparam 'stakeSplitTarget') @@ -202,13 +202,6 @@ describe('Wallet transaction tests', () => { }) ); } - // Undelegated change - expect(tx.vout[11]).toStrictEqual( - new CTxOut({ - script: '76a914f49b25384b79685227be5418f779b98a6be4c73888ac', - value: 475009989980, - }) - ); await checkFees(wallet, tx, MIN_FEE_PER_BYTE); }); @@ -229,16 +222,15 @@ describe('Wallet transaction tests', () => { scriptSig: '76a914f49b25384b79685227be5418f779b98a6be4c73888ac', // Script sig must be the UTXO script since it's not signed }) ); - // Cold Stake split outputs - expect(tx.vout).toHaveLength(21); - // Undelegated dust change (<1 coin, too small to stake) - const fees = await checkFees(wallet, tx, MIN_FEE_PER_BYTE); - expect(tx.vout[20]).toStrictEqual( + // The 'after split' output + expect(tx.vout[0]).toStrictEqual( new CTxOut({ - script: '76a914f49b25384b79685227be5418f779b98a6be4c73888ac', - value: 10000000 - fees, + script: '76a97b63d114291a25b5b4d1802e0611e9bf724a1e57d9210e826714f49b25384b79685227be5418f779b98a6be4c7386888ac', + value: 50009999246, }) ); + // Cold Stake split outputs + expect(tx.vout).toHaveLength(20); }); it('creates a t->s tx correctly', () => {