dtr-org / unit-e

A digital currency for a new era of decentralized trust
https://unit-e.io
MIT License
45 stars 15 forks source link

Incorrect error message for "spent" tx after reorg #962

Closed castarco closed 5 years ago

castarco commented 5 years ago

Here, the transaction b2fe27c7e3986803749ef0ed0b5afcc5ae2de60270f52de2bc5486412490113c was created when proposing the block 68a60775ac89520b1ab9f8fa6783fb28462529324affe6567c9bd8e34e65045a, which was discarded after some time to follow a longer chain [c14175679a5db141206b01def9841eba0e3056a7d0fd7b0333309d5aadfbe541, c77ebab0ae95cbc51508ffd4b89ee8f7373c44b26447986b7e8f246f223b7caa].

Then, the node proposed again using the same funds were used to stake for 68a60775ac89520b1ab9f8fa6783fb28462529324affe6567c9bd8e34e65045a, which should be OK since we are in a fork were these funds were not used. But then the node shows us an error message, telling us that there's a conflict (the coinbase transaction b1f8f4eb440799573f4fcce00f634444cf8e4666ff147676bee7ae9db66d10e7 conflicts with b2fe27c7e3986803749ef0ed0b5afcc5ae2de60270f52de2bc5486412490113c).

As far as I understand, we should not see this kind or error message in the logs since everything was working as expected.

2019-04-12 08:21:58 [            ] CreateNewBlock: block weight=1010294 txs=74 fees=5198204 sigops=2620
2019-04-12 08:21:58 [            ] ERROR: TestBlockValidity: CChainState::ConnectBlock: bad-stake-not-eligible, STAKE_NOT_ELIGIBLE (code 16)
2019-04-12 08:21:58 [            ] UpdateTip: new best=68a60775ac89520b1ab9f8fa6783fb28462529324affe6567c9bd8e34e65045a height=7658 version=0x00000001 log2_work=66.853437 tx=640756 date='2019-04-12 08:22:0
0' progress=1.000000 cache=4.5MiB(33307txo)
2019-04-12 08:21:58 [            ] AddToWallet b2fe27c7e3986803749ef0ed0b5afcc5ae2de60270f52de2bc5486412490113c  new
2019-04-12 08:22:13 [            ] UpdateTip: new best=c14175679a5db141206b01def9841eba0e3056a7d0fd7b0333309d5aadfbe541 height=7657 version=0x00000001 log2_work=66.853217 tx=640755 date='2019-04-12 08:21:52' progress=1.000000 cache=4.5MiB(33307txo)
2019-04-12 08:22:13 [            ] UpdateTip: new best=c77ebab0ae95cbc51508ffd4b89ee8f7373c44b26447986b7e8f246f223b7caa height=7658 version=0x00000001 log2_work=66.853437 tx=640820 date='2019-04-12 08:22:00' progress=1.000000 cache=4.4MiB(32734txo)
2019-04-12 08:22:13 [            ] UpdateTip: new best=e83e74c1fbeb2432f88f9c7bc3e5799905d1fd8eb54e78ba61c1587579a0cbe3 height=7659 version=0x00000001 log2_work=66.85366 tx=640940 date='2019-04-12 08:22:16' progress=1.000000 cache=4.3MiB(31666txo)
2019-04-12 08:22:13 [            ] AddToWallet b2fe27c7e3986803749ef0ed0b5afcc5ae2de60270f52de2bc5486412490113c  
2019-04-12 08:22:22 [            ] UpdateTip: new best=071ba4b75e6fc96e9243d1ec6f9150ae9ed8a92505a4ff9151b97da7caadb20a height=7660 version=0x00000001 log2_work=66.853879 tx=640987 date='2019-04-12 08:22:24' progress=1.000000 cache=4.2MiB(31255txo)
2019-04-12 08:22:26 [            ] CreateNewBlock: block weight=1662352 txs=127 fees=8557619 sigops=3833
2019-04-12 08:22:26 [            ] UpdateTip: new best=5a6825909d8f302b34c1a7ba1b6ac8da366cd85fb1b9347dd111f3e8ead2beae height=7661 version=0x00000001 log2_work=66.854098 tx=641115 date='2019-04-12 08:22:28' progress=1.000000 cache=4.2MiB(31011txo)
2019-04-12 08:22:26 [            ] Transaction b1f8f4eb440799573f4fcce00f634444cf8e4666ff147676bee7ae9db66d10e7 (in block 5a6825909d8f302b34c1a7ba1b6ac8da366cd85fb1b9347dd111f3e8ead2beae) conflicts with wallet transaction b2fe27c7e3986803749ef0ed0b5afcc5ae2de60270f52de2bc5486412490113c (both spend 7f0742595f241170350c1b9777c9d6df6ff8c7c533961e99ec1f014825169f15:1)
2019-04-12 08:22:26 [            ] AddToWallet b1f8f4eb440799573f4fcce00f634444cf8e4666ff147676bee7ae9db66d10e7  new
2019-04-12 08:22:27 [            ] UpdateTip: new best=d03ddee89227bce93608f67cdc94c2fa0c427bbff43f7894e4ee84777ada3cfb height=7662 version=0x00000001 log2_work=66.854317 tx=641128 date='2019-04-12 08:22:28' progress=1.000000 cache=4.2MiB(31182txo)

To Reproduce No clear way to reproduce the issue, it depends on randomness.

Expected behavior The error message should not appear at all.

Environment Testnet (early stages) Ubuntu 18.04, but this is not really relevant.

Nizametdinov commented 5 years ago

I would say it's not an error message. It can be prevented by removing transactions from the wallet after reorgs. But I'm not sure we should do it.

cmihai commented 5 years ago

We can remove coinbase transaction A from the wallet once another transaction B which spends A's stake has been finalized. Otherwise we risk A's fork becoming active again.

Nizametdinov commented 5 years ago

Otherwise we risk A's fork becoming active again.

In that case CWallet::BlockConnected will be called again and A will be added to the wallet.

thothd commented 5 years ago

Well I think it's more in terms of usability the wallet should only provide what's on your main chain, even if you still keep this block.

castarco commented 5 years ago

My doubt here is if there's any reason why this was done that way before. I mean, why after reorg the wallet is not purged by default from transactions created in the old fork? Is it because we already have this method that does it "on demand"? Or is there any other reason?

I'm looking into it to see if it makes sense to apply this idea of removing UTXOs from the wallet proactively without waiting for conflicts.

Nizametdinov commented 5 years ago

My doubt here is if there's any reason why this was done that way before.

Because usually, wallet handles transactions created by the user. It would be strange to delete user's transactions after switching to another fork.

But for coinbase transactions, it makes more sense since they cannot be included in other blocks.

thothd commented 5 years ago

Yeah it’s a side effect of the staking wallet. In Bitcoin the wallet is of course not remotely close to create coinbase transactions, not even the entire codebase. Yeah we should definitely handle it specially and I’m not surprised,

castarco commented 5 years ago

True. What comes to my mind now is if the maturity constrains are enough to avoid having to care about the following case (the problem here is that we still don't have definitive values):

In the case of a coinbase transaction that has to be discarded, whose outputs are used to create regular transactions... what should the Wallet do with those regular transactions?

castarco commented 5 years ago

I'll close this issue for now, as the remaining doubts deserve a proper issue... and just in case anyone really cares about that.

I don't see a real/important problem here, since it's already expected that re-orgs could lead to discarding transactions. It's true that we could slightly improve the node's behaviour in some cases, but in my opinion the effort isn't worth it because such cases will be very rare.