fix: ensure minimum non-dust amount as change output on regtest

[janniks]

• Adds DUST_UTXO_LIMIT to the to_spend amount for selecting utxos of the BitcoinRegtestController -- this ensures there will always be at least DUST_UTXO_LIMIT as a change output.
  • we now don't pass the total_spend to the serialize function (which selects utxos), but rather a tx_cost.
  • if we want to force a change output the target amount (for selecting utxos) is set to the tx_cost plus DUST_UTXO_LIMIT.
  • for actual "spending" on the transaction the tx_cost is still used, to ensure a change output >= DUST_UTXO_LIMIT if we force one.
  • Also updates estimated tx size for block commit tx to 380
• Closes Ensure bitcoin ops always include a change address #4992

---

This might be a quick, good enough fix -- or are there implications for non-regtest code paths?
lmk if anybody has ideas on writing a test for this.

[janniks]

Does this make sense, my brain can't think this through right now 🫠

[obycode]

This looks like it should work to me. Do you think you could add a unit test?

[obycode]

This looks like it should work to me. Do you think you could add a unit test?

There wasn't one before, so I think I'd merge this without, but if it's not terrible, it would be great to add.

[wileyj]

@janniks there are several test failures that seem to be a result of this change, ex: https://github.com/stacks-network/stacks-core/actions/runs/10102415322/job/27939605277?pr=5008

do these failing tests pass locally?

[jbencin]

I think you have to make changes inside serialize_tx() also. You are adding everything up into a single variable, total_to_spend, and passing that to serialize_tx(), but the decision to make a change output in that function is made by:

     let value = total_consumed - total_to_spend;
     if value >= DUST_UTXO_LIMIT {
       // Make change output
     }

total_consumed here is just the minimum UTXO set that's larger than total_to_spend, so it's still possible for the difference to be less than DUST_UTXO_LIMIT

[obycode]

Good call Jeff 👍. After another look, I agree, it does need some changes.

[janniks]

Lol, yeah this is why I initially closed the PR again. Thanks @jbencin -- I think I'll edit the utxo collection condition in serialize, hoping this is okay for the bitcoin regtest controller.

[jbencin]

I think you should have all the logic for this inside serialize_tx(). Here's an idea that I think would work: Add a force_change_output parameter to serialize_tx(), and if set, make sure to take a little extra from the UTXO set. See this commit: jbencin@416ff55

[janniks]

Update based on @jbencin suggestion. Please double check the logic:

• we now don't pass the total_spend to the serialize function, but rather a tx_cost.
• if we want to force a change output the target is set to the tx_cost plus DUST_UTXO_LIMIT.
• for actual "spending" on the transaction the tx_cost is still used, to ensure a change output if we force one.

Won't be able to get a test done in time, should be small enough to review instead.

[jbencin]

This looks correct to me now, but integration tests are still failing

[jcnelson]

Seems several CI tests are still failing

[janniks]

cargo nextest run is successful for me locally (apart from two flaky tests, that seem unrelated)

[jbencin]

cargo nextest run is successful for me locally (apart from two flaky tests, that seem unrelated)

It's the bitcoind integration tests that are failing, and those are skipped by default. If you want to run one of the failing tests locally, do this:

RUST_BACKTRACE=1 BITCOIND_TEST=1 cargo test -- --include-ignored stx_delegate_btc_integration_test

[wileyj]

and to emulate how the test will run in ci, something like this (which uses nextest):

RUST_BACKTRACE=full BITCOIND_TEST=1 cargo nextest run --no-capture \
    --verbose \
    --tests \
    --workspace \
    --run-ignored all \
    --bin stacks-node \
    --package stacks-node \
    --retries 0  \
    --no-fail-fast \
    -E "test(=tests::neon_integrations::stx_delegate_btc_integration_test)"

I've run a few the failing tests locally, and this is the error i see:

ERRO [1722285437.277667] [testnet/stacks-node/src/burnchains/bitcoin_regtest_controller.rs:1876] [tests::nakamoto_integrations::stack_stx_burn_op_integration_test] Bitcoin RPC failure: transaction submission failed - Network("Bitcoin RPC: status(500) != success, body is 'Object {\"error\": Object {\"code\": Number(-22), \"message\": String(\"TX decode failed. Make sure the tx has at least one input.\")}, \"id\": String(\"stacks\"), \"result\": Null}'")

with the failing tests being:

    tests::neon_integrations::stx_delegate_btc_integration_test
    tests::neon_integrations::stx_transfer_btc_integration_test
    tests::neon_integrations::stack_stx_burn_op_test
    tests::neon_integrations::vote_for_aggregate_key_burn_op_test
    tests::epoch_25::microblocks_disabled
    tests::nakamoto_integrations::vote_for_aggregate_key_burn_op
    tests::nakamoto_integrations::stack_stx_burn_op_integration_test

[janniks]

Updated tx-size estimates for ops in the regtest controller -- those were too low in cases and didn't find matching UTXOs. Thanks @obycode for finding.

Ready for rereview assuming tests pass 🫡

@kantai Do we have exact values for these estimates, or do you have a good way of figuring them out?

[obycode]

Yup, this looks good to me, but I would appreciate someone who is more familiar with the bitcoin transactions checking the sizes.

[wileyj]

two tests still failing (locally) with same error:

tests::neon_integrations::stack_stx_burn_op_test 
tests::nakamoto_integrations::stack_stx_burn_op_integration_test 

    ERRO [1722355798.568427] [testnet/stacks-node/src/burnchains/bitcoin_regtest_controller.rs:1882] [tests::neon_integrations::stack_stx_burn_op_test] Bitcoin RPC failure: transaction submission failed - Network("Bitcoin RPC: status(500) != success, body is 'Object {\"error\": Object {\"code\": Number(-22), \"message\": String(\"TX decode failed. Make sure the tx has at least one input.\")}, \"id\": String(\"stacks\"), \"result\": Null}'")

[obycode]

I think there are two unknowns that need to be investigated:

1. Are those maximum tx size values accurate?
2. Could it be that the change adds an extra input utxo which causes the size to be bigger than expected? How should that be handled?

[janniks]

two tests still failing (locally) with same error:

tests::neon_integrations::stack_stx_burn_op_test 

tests::nakamoto_integrations::stack_stx_burn_op_integration_test 

    ERRO [1722355798.568427] [testnet/stacks-node/src/burnchains/bitcoin_regtest_controller.rs:1882] [tests::neon_integrations::stack_stx_burn_op_test] Bitcoin RPC failure: transaction submission failed - Network("Bitcoin RPC: status(500) != success, body is 'Object {\"error\": Object {\"code\": Number(-22), \"message\": String(\"TX decode failed. Make sure the tx has at least one input.\")}, \"id\": String(\"stacks\"), \"result\": Null}'")

Thx for these, trying to figure out what's wrong here. Maybe the StackStx estimate is also off, but it seems to be off by a large margin, so I'm not sure..

[janniks]

Updated to only force the change output for block commits (according to the from_tx parse code it seems that's the only type which really needs the change output)

[obycode]

👍

[jbencin]

LGTM

[wileyj]

looks good, thanks @janniks

still see this failure : tests::signer::v0::empty_sortition but it seems unrelated to this PR