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

slimcoind getaddressesbyaccount and slimcoind listreceivedbyaddress reliability #221

Open
buhtignew opened this issue Nov 29, 2024 · 7 comments
Labels
enhancement New feature or request

Comments

@buhtignew
Copy link
Collaborator

buhtignew commented Nov 29, 2024

The following text was originally written as replay to your last remark for the issue #204.

However I've realized that I would be largely off-topic there so I've decided to close that issue and to open this one.
_
I apologize for it seems like I'm always trying to defend my old wallet.dat file :-) but also in this case I'm not sure it to be broken.

I've tested these days on the new wallet.dat I have and discovered that slimcoind creates the change addresses that are not visible by pacli and neither with slimcoind getaddressesbyaccount "" (the same for any other account displayed in slimcoind listaccounts output) nor with slimcoind listreceivedbyaddress 0 true | grep ADDRESS, but are visible with slimcoind validateaddress THE_SAME_ADDRESS command.

The steps to reproduce the issue are the following:

  • slimcoind sendtoaddress ANY_ADDRESS_HERE 1;
  • pacli transaction show TX_ID_FROM_THE_PREVIOUS_STEP -s;
  • pick the change address from the previous output;
  • slimcoind listaccounts;
  • slimcoind getaddressesbyaccount ANY_ACCOUNT_FROM_THE_PREVIOUS_STEP | grep CHANGE_ADDRESS - no results;
  • slimcoind listreceivedbyaddress 0 true | grep CHANGE_ADDRESS - no results;
  • slimcoind validateaddress CHANGE_ADDRESS - slimcoind sees that address and considers it as belonging to the current wallet.dat.

I assume, as you mentioned, bitcoind had the same issue in the past and probably also for that reason people were using pywallet.py, for instance, to dump the wallet.dat (because bitcoind was not providing the dumpwallet command at that epoch as slimcoind is not providing it now). Probably slimcoind code was forked at that stage so we are experiencing now the same kind of issue.

I was wondering whether we can include the pywallet.py into our pacli package until we upgrade the old slimcoind code and to use its results instead of the original slimcoind commands' so to feed pacli with more reliable data, but unfortunately it seems like the versions of that program available in the internet are obsolete (the most recent version I've found is 3 years old) and they are all relying on the deprecated bsddb package.
However there is a more modern bsddb3 package although not compatible with the existing pywallet.py versions but, if I've got it right, it provides the possibility to read the Berkley DB data from the wallet.dat file, that is basically what slimcoind should do and we'd need.
(There are also other utilities to read a file created with Berkley DB, for instance by installing the library libdb1-compat that was making part of Ubuntu18.04 one would be able to dump the Berkley DB file by running the db_dump[BDB_version_here] command and there is a solution I've found here, utilizing other libraries).

In any case I don't know whether at this stage of our development it would make sense for us to dedicate our time to this matter.

Maybe we can basically mention somewhere in our documentation or in commands' help that the transactions created by slimcoind and not by pacli may not be displayed by pacli correctly so one should avoid using slimcoind in place of pacli.

@buhtignew buhtignew added the enhancement New feature or request label Nov 29, 2024
@buhtignew buhtignew changed the title slimcoind getaddressbyaccount and slimcoind listreceivedbyaddress reliability slimcoind getaddressesbyaccount and slimcoind listreceivedbyaddress reliability Nov 29, 2024
@d5000
Copy link

d5000 commented Dec 2, 2024

I have indeed been able to reproduce the steps and it seems you are correct. Good find. I have also found some Stackexchange conversations related to this problem.

The basic problem seems to be that Bitcoin stores change addresses in a separate category and they are not accessed by the account-related commands and also not by listreceivedbyaddress.

Solving this by adding pywallet would solve the problem for the address list commands and the -w options of some variants of transaction list, but transaction list would still not be working perfectly, because listtransactions (command used for the transaction lists, with the exception of the -b and -x variants) seems also not to show transactions from change addresses to other change or non-wallet addresses. I've just tested that, but maybe I've to investigate it more.

I've to think about if its worth to spend time on this thus. In theory only one function of the pacli code could be changed and use pywallet instead of listreceivedbyaddress for the list of wallet addresses.

There is also an intermediate possible solution: a script transferring the change addresses which are not detected by address list into the extended config file. This would be less comfortable but perhaps enough, and it would be better from a security perspective, as the pywallet code potentially creates ways to expose private keys.

I got also another idea: without pywallet, it would be perhaps possible to detect the change addresses with a command using listtransactions which would detect the first transaction to a change address (i.e. the one created by sendtoaddress) and then check each output address with validateaddress. This would be another way that could be used to store the addresses in the extended config file, but would be too slow to use it "on the fly" in a command like transaction list -w. Considering the required development time this would be the easiest option - probably less than an hour to code that - , so I perhaps would simply go for it before deciding if adding pywallet is an option.

@buhtignew
Copy link
Collaborator Author

I agree that the last option would be straightest one.

By other hand I was also thinking that the implementation of the pywallet library (or of another tool to read the wallet.dat directly) would open us the way to let people using pacli without launching slimcoind , at least for some command, or also while slimcoind is launched but not ready to receive commands yet (and in production the launch of slimcoind would require much more time than on the testnet).

However I understand that the time to code things can't be unlimited.

@d5000
Copy link

d5000 commented Dec 3, 2024

By other hand I was also thinking that the implementation of the pywallet library would open us the way to let people using pacli without launching slimcoind

This "dependency" on slimcoind is a structural design decision of the pacli application. Once you select a "provider" via the pacli.conf file, and this "provider" is slimcoind (the setting value corresponding to slimcoind is slm_rpcnode), then it will always try to connect to slimcoind at startup, no matter which command you use.

And pywallet would only allow very few "static" commands to work, like the address list variants which do not show the current balance but only labels and addresses. Most commands need to have "a look" on the on-chain data, and for these slimcoind is needed. An alternative which was explored by the Peercoin folks is using block explorers instead.

Labels and named addresses can even be shown without any "provider" dependency. But again, this would need structural changes to pacli.

Anyway that's sort of OT here (I've still not taken a decision, that can take time in this case), I only want to avoid false hopes. The problem I have with adding pywallet directly to the package is mostly the security issue (potential exposure of private keys). Currently I think the benefits would not be high enough to counter that.

@buhtignew
Copy link
Collaborator Author

buhtignew commented Dec 4, 2024

Maybe we can limit the pywallet functionality by inhibiting its ability to access the private keys?

@buhtignew
Copy link
Collaborator Author

A funny discovery I've just made: it's possible to set the "outside" (change) address that is not visible by pacli using the address list command as the main one using address set -a CHANGE_ADDRESS command.

@d5000
Copy link

d5000 commented Dec 10, 2024

Just FYI: I've already implemented partly the solution to search for the change addresses via the listtransactions command (search the transactions made by the slimcoin client).

I've currently implemented it only for address list -c, and with the new flag which searches for change addresses the command takes approximately two times the original command's duration. But it may be possible to optimize that, above all if the approach is used inside of a command like transaction list -b -w where it would make most sense, because listtransactions is already used in these commands so probably both informations (change addresses and burn/gateway transactions) can be extracted from a single operation. For this reason I've still not uploaded the change and it may be possible that we don't need an approach based on pywallet at all.

A funny discovery I've just made: it's possible to set the "outside" (change) address that is not visible by pacli using the address list command as the main one using address set -a CHANGE_ADDRESS command

Yes, when you create a label of an address it will become part of the addresses shown by address list, regardless of it being part of the "recognized wallet addresses" or not. The balances of both coins and tokens can then also be retrieved.

Basically once you know the address, pacli can work with it. The complicated thing is to create a list of all change addresses with slimcoind commands, and thats where I'm currently working on.

Maybe we can limit the pywallet functionality by inhibiting its ability to access the private keys?

The code could be manipulated, but that would need more work, and as written above, it's likely we don't need it.

@buhtignew
Copy link
Collaborator Author

Ok, looking forward to the upgrade.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants