Closed tawmaz closed 1 year ago
It appears to be only an issue with pocketcoin-qt and the RecentRequestsTableModel class which is only used by pocketcoin-qt. pocketcoind was able to load my wallet correctly, fully sync, and display my balance. I will check to see if there are some changes which did not get merged over from bitcoin.
In the 0.21 branch there appears to have been some big changes to walletmodel.cpp which is in the code stack of this crash in this commit here: https://github.com/pocketnetteam/pocketnet.core/commit/87dc9c573ee651cb6472b7362a8ef3b05c0f420c
The code appears to be different from both bitcoin core and 0.20 branch.
In the 0.21 branch there appears to have been some big changes to walletmodel.cpp which is in the code stack of this crash in this commit here: 87dc9c5
The code appears to be different from both bitcoin core and 0.20
The code is absolutely the same as in bitcoin core at af591f2 because there were no changes from us in walletmodel.cpp except renaming all to pocket.
Also, i'm unable to reproduce this, my wallet is loaded well. I've tried both wallets created with 0.20 and 0.21 and both were loaded well. However, i used fresh created wallets, probably this is the difference. @tawmaz, can you provide more info on how i can reproduce this?
Attached the testnet wallet file which was used to repro this.
The problem is in serialization for SendCoinsRecipient class.
e020490188648a1444119c6115241b2cc506b5b0 introduced serialization changes and removed serializing field sPaymentRequest
from SendCoinsRecipient class for 0.20. Later, for 0.21 SendCoinsRecipient class was extracted from walletmodel.h to different file and sPaymentRequest
was returned because it is situated in bitcoin upstream. Absence of this field is the reason why error doesn't occur in 0.20. @tawmaz, can you remember what was the reason for removing sPaymentRequest
from serialization?
However, in 0.21 sPaymentRequest
seems to be no longer meaningful and is used just to not break serialization of older wallets, but i don't think we could safely just remove it because it is deserialized before auth_merchant_str
and so deleting sPaymentRequest
will prevent from correct deserializing auth_merchant_str
and any potentially new added field after it.
Thus, if we really can't handle sPaymentRequest
and even don't need it, we should also remove auth_merchant_str
and take care of not adding some more fields to wallet recipients that can be a shotgun in the future.
Notes:
sPaymentRequest
from serialization for 0.21 here results in successful wallet loading.@andyoknen said he can test this issue's reproducibility on a lot of different wallets, so we can be sure that this is not a specific problem to the wallet @tawmaz attached.
It looks like it was my mistake when porting to the new serialization functions. It would be worth enabling qt/test/wallettests.cpp which would catch this issue.
One idea would be to perform the deserialization in a try-catch, and if we encounter the overrun serialization issue perform the incorrect serialization from 0.20.19 to update the wallet to the new format. Let me know if you want me to retake this issue @lostystyg
Pocketcoin-qt fails with exception when attempting to load wallet, with message "CDataStream::read(): end of data: iostream error". Exception occurs in function RecentRequestsTableModel::addNewRequest when serializing requests from the wallet. Error is not seen with 0.20.19 build.
Stack below: