Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change position not null, but tx has only one output #16

Open
remyers opened this issue Jan 5, 2024 · 5 comments
Open

change position not null, but tx has only one output #16

remyers opened this issue Jan 5, 2024 · 5 comments

Comments

@remyers
Copy link

remyers commented Jan 5, 2024

Bitcoin version: master (c80f57ba5)
coin-selection-simulation: main (c604fad)

I'm getting an assert because the change position is not null, but the tx has only one output. I'm not sure how to debug this further. Any suggestions?

~/github/coin-selection-simulation$ sudo scripts/simulation.py --scenario scenarios/bustabit-2019-2020.csv ~/github/bitcoin/test/config.ini results/
2024-01-05T15:52:09.037000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_coin_sel_sim_qoh2dde6
2024-01-05T15:52:09.300000Z TestFramework (INFO): Based on branch master(c80f57ba575af96890f185765a53a62ef58ef2c8)
2024-01-05T15:52:09.300000Z TestFramework (INFO): This simulation's Unique ID: 9a9224c4d27f42b1891b27328c20a3f6
2024-01-05T15:52:09.331000Z TestFramework (INFO): Mining blocks for node0 to be able to send enough coins
2024-01-05T15:52:10.236000Z TestFramework (INFO): Simulating using scenario: bustabit-2019-2020
2024-01-05T15:52:10.236000Z TestFramework (INFO): 0 operations performed so far
Traceback (most recent call last):
  File "/home/remyers/github/coin-selection-simulation/scripts/simulation.py", line 582, in <module>
    CoinSelectionSimulation().main()
  File "/home/remyers/github/coin-selection-simulation/scripts/framework.py", line 195, in main
    self.run()
  File "/home/remyers/github/coin-selection-simulation/scripts/simulation.py", line 540, in run
    assert len(dec["tx"]["vout"]) == 2
AssertionError
[node 0] Cleaning up leftover process

I added some debugging lines and it looks like before the assert the state is:

event 2
success= 1
fee= 600
change_pos= 0

event 4
change_aps= 1
success= 1
fee= 600
change_pos= 0

len(dec["tx"]["vout"])= 1
change_pos= 0

dec["tx"]={'txid': '6ab21c3263e3c39d9b4abf1e34d40503f8e171ea5f089962ad81e7d0070c67c5', 'hash': '6ab21c3263e3c39d9b4abf1e34d40503f8e171ea5f089962ad81e7d0070c67c5', 'version': 2, 'size': 82, 'vsize': 82, 'weight': 328, 'locktime': 0, 'vin': [{'txid': '453c271c91d8f159d79c73f314069c9a8cd5aa769c591e9b42c66f8e1ff59934', 'vout': 0, 'scriptSig': {'asm': '', 'hex': ''}, 'sequence': 4294967293}], 'vout': [{'value': Decimal('0.00329000'), 'n': 0, 'scriptPubKey': {'asm': '0 727e19b73e721b43568abe335af127e957432da0', 'desc': 'addr(bcrt1qwflpnde7wgd5x452hce44uf8a9t5xtdqh2mkp9)#utql76pz', 'hex': '0014727e19b73e721b43568abe335af127e957432da0', 'address': 'bcrt1qwflpnde7wgd5x452hce44uf8a9t5xtdqh2mkp9', 'type': 'witness_v0_keyhash'}}]}

@remyers
Copy link
Author

remyers commented Jan 9, 2024

It appears that an event type 4 (aps_create_tx_internal) sets change_pos to 0 if the APS (avoid partial spends) attempt finds a solution without a change output. Should the simulation be run without APS? otherwise it seems like 0 is a valid change position result for a aps_create_tx_internal

@remyers
Copy link
Author

remyers commented Jan 9, 2024

Actually it seems like when event type 2 ( normal_create_tx_internal ) has no change output, but returns change_pos as 0 that that is triggering the assert. I'm still not sure why others haven't seen this problem. Did something changed in bitcoin?

@achow101
Copy link
Owner

Ah, that looks like a bug in Bitcoin Core. The return value for change position was changed from an int to an optional, but it is still being output as an int in a few places, like here. The conversion seems to be wrong, I think that's supposed to be a -1 rather than 0.

@remyers
Copy link
Author

remyers commented Jan 11, 2024

Thanks for the confirmation! I will suggest these fixups in @murchandamus 's coingrinder PR after it is rebased. It doesn't seem worth a stand alone PR to core. In the mean time I can make the changes locally so my simulations run.

@murchandamus
Copy link
Contributor

I’d expect the fix to an outright bug like that get merged easily, and it might be better to fix separately, since CoinGrinder doesn’t seem to be on many people’s priorities, it’s been open since July.

achow101 added a commit to bitcoin/bitcoin that referenced this issue Jan 23, 2024
… change pos

d55fdb1 Move TRACEx parameters to seperate lines (Richard Myers)
2d58629 wallet: fix coin selection tracing to return -1 when no change pos (Richard Myers)

Pull request description:

  This is a bugfix for from when [optional was introduced](758501b)  for `change_pos` in the wallet. When optional `change_pos` is unset, we should return -1 and not 0.

  I added two new checks to the `test/functional/interface_usdt_coinselection.py` which adds coverage for the situations when `normal_create_tx_internal` and `aps_create_tx_internal` events occur with no change.

  You can reproduce this bug using the coin-selection-simulation scripts as described in [issue #16](achow101/coin-selection-simulation#16). You can also run the `interface_usdt_coinselection.py` test  without the changes to `wallet/spend.cpp`.

ACKs for top commit:
  0xB10C:
    ACK d55fdb1
  achow101:
    ACK d55fdb1
  murchandamus:
    ACK d55fdb1

Tree-SHA512: 6efac3b756bdf51debbcb759dc3c4b7a4304626bc047b70025cec02f3a04937ace7712e9558ac71e560fd136005a98c518ac5bb4b90c3282d776beccd0de9749
remyers pushed a commit to remyers/bitcoin that referenced this issue Jan 27, 2024
…when no change pos

d55fdb1 Move TRACEx parameters to seperate lines (Richard Myers)
2d58629 wallet: fix coin selection tracing to return -1 when no change pos (Richard Myers)

Pull request description:

  This is a bugfix for from when [optional was introduced](bitcoin@758501b)  for `change_pos` in the wallet. When optional `change_pos` is unset, we should return -1 and not 0.

  I added two new checks to the `test/functional/interface_usdt_coinselection.py` which adds coverage for the situations when `normal_create_tx_internal` and `aps_create_tx_internal` events occur with no change.

  You can reproduce this bug using the coin-selection-simulation scripts as described in [issue bitcoinops#16](achow101/coin-selection-simulation#16). You can also run the `interface_usdt_coinselection.py` test  without the changes to `wallet/spend.cpp`.

ACKs for top commit:
  0xB10C:
    ACK d55fdb1
  achow101:
    ACK d55fdb1
  murchandamus:
    ACK d55fdb1

Tree-SHA512: 6efac3b756bdf51debbcb759dc3c4b7a4304626bc047b70025cec02f3a04937ace7712e9558ac71e560fd136005a98c518ac5bb4b90c3282d776beccd0de9749
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants