wallet: bugfix; if datadir has a trailing '/' listwalletdir would strip lead char of walletname

[Saibato]

fixes: #19928

[luke-jr]

Looks a bit fragile, but I guess better than nothing, and there's a plan to replace this with a less-fragile version when Boost is bumped anyway. utACK

[luke-jr]

Liked the first version better...

[Saibato]

Liked the first version better...

me 2 , but failed with some standard cases, i guess there are better way's to do this?
What we need is a patch without trailing'/' siggh...

[promag]

Could GetWalletDir() return a "clean" path?

[Saibato]

Could GetWalletDir() return a "clean" path?

That would be nice to have.
It defaults in the error case we tackle here to GetDataDir() we would need then in GetWalletDir() to strip all trailing chars off type fs::path::preferred_separator. there.
But that could also tangle deep into other parts where we need GetWalletDir.

Tested that a bit, but then I found that path could be also like "/tmp////...\.////test...///\/////" a hell
to strip correctly,without boost > 1.47 siggh.
To go for size was so far what worked with all sorts of path and syminks.
And here we only build somewhat symlinks to the real wallet names an path .

At least it will return at minimum a single "." so it will not empty for the back() string call.

edit@saibato

@promag this alternative patch just in GetWalletDir() would also work to mitigate the error. like suggest.
Not sure whats better and if there no side effects to other parts?
So far i counted 7 references.

At least some fewer lines so less error?

diff --git a/src/wallet/walletutil.cpp b/src/wallet/walletutil.cpp
index e4c72aed9..afd915f82 100644
--- a/src/wallet/walletutil.cpp
+++ b/src/wallet/walletutil.cpp
@@ -20,6 +20,9 @@ fs::path GetWalletDir()
        }
    } else {
        path = GetDataDir();
+        if (path.string().back() == fs::path::preferred_separator) {
+            path = path.parent_path().string();
+        }
        // If a wallets directory exists, use that, otherwise default to GetDataDir
        if (fs::is_directory(path / "wallets")) {
            path /= "wallets";

[tryphe]

Concept ACK. Prefer the patch above rather than the active PR. Logic is easier on the eyes and we need it everytime we call GetWalletDir(). As promag mentioned, it would be nice to know why the underlying function differs between systems, rather than adding this logic on. If it's necessary, I think this is a good solution, but if other path functions need this logic, would prefer something like a GetCleanPath static function.

[luke-jr]

My "obsolete" comments above remain unaddressed. Would like a test to reproduce it if possible - I have been unable to do so.

[Saibato]

@tryphe

GetCleanPath static function.

hmm,,,, good idea, seams reasonable.

Btw; where u also able to reproduce the bug with current master i.e.
with
cc @luke-jr

Would like a test to reproduce it if possible

mkdir /tmp/wallet_test
bitcoind -daemon -datadir=/tmp/wallet_test -connect=127.0.0.1
sleep 2
bitcoin-cli -datadir=/tmp/wallet_test createwallet bitcoin1 
sleep 2
bitcoin-cli  -datadir=/tmp/wallet_test createwallet bitcoin2 
sleep 2
bitcoin-cli  -datadir=/tmp/wallet_test stop 
sleep 2
bitcoind -daemon -datadir=/tmp/wallet_test/ -connect=127.0.0.1 
sleep 2
bitcoin-cli  -datadir=/tmp/wallet_test listwalletdir
sleep 2
bitcoin-cli  -datadir=/tmp/wallet_test stop

Output should look like

{
  "wallets": [
    {
      "name": "itcoin2"
    },
    {
      "name": "itcoin1"
    },
    {
      "name": ""
    }
  ]
}

if u open it the next time with i.e. bitcoind -daemon -datadir=/tmp/wallet_test/// even more chars are missing.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• Fix crashes and infinite loop in ListWalletDir() #18095 (Fix crashes and infinite loop in ListWalletDir() by uhliksk)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[Saibato]

TYI: This behavior is AFAICS only with mainnet nodes, what makes it even more annoying since u can not test it easy before it hits u in cli or qt gui with real coins.

Might be not the right place here, but when I see 450Mil inflow in something like this with bold speech;
i ask myself are they serious not to fund before that 1000 devs that feel a duty to lock at shit like this ( this is core not a an alt)?

Not that this here is at first glance a big deal ( but who knows what might be else there?)

I ask myself have they any clue or time to test and are there really no devs and users out there that can just confirm this easy test bash and clickbait this PR or do anything else but let it not happen that this kind of untested footguns are out there and not get backported fast.

This kind of shit can in my view really scare ppl away from core.

If i would see my precious wallets get fancy names behind my back in the reference implementation only if I just do one typo, that btw is not even wrong but an allowed way to type dir names. I would freak out.

Knowing that some of the reference code is probably copy pasted to a ton of of alts and other app's .
Rant stop here. but hey this PR sits 11 days here,

I know TAPROOT and Wall Street happiness and doxing Tor shit has prio, but hey if boldness top vetted super reviewed best coin talk and reality differ that much, i wonder wonder wonder.

TYI: I do this in my spare time ( ok, i have lot ) and not with a 100 devs team but since i locked in this some month ago i already had found 7 mayor things that where utterly wrong.

So pls give me a hint that this is work in progess and not only https://github.com/bitcoin/bitcoin/milestone/45

[maflcko]

This kind of shit can in my view really scare ppl away from core.

The bug only exists in the master branch, which is known to have bugs and should not be used in production. It is good that this issue was found. There won't be a 0.21 release without this bug fixed, which is why I assigned that milestone. You can find the schedule for the release here: #18947

[Saibato]

Thx @MarcoFalke but tyi, the bug exits in every release and some in alt projects that followed core since we have multiple wallets 18,x,19.x,,20.x, I build for test from source to compare.

[meshcollider]

Tested ACK 7be117c

It seems the problem only appears using mainnet as mentioned.

This seems to be because only the trailing separators cause it (doesn't matter if there are multiple separators mid-path), and when using regtest or testnet, the path is automatically appended with the appropriate sub-directory, and no slash. Thus another fix would just be to always append /. to the path. Not sure which hack is uglier.

[hebasto]

Concept ACK.

[hebasto]

Now we have boost 1.58 (#19667).

[luke-jr]

Not in supported stable versions.

[Saibato]

I guessed it might be backported at least to 0.19 and in 0.18 the bug is only int qt gui so 1.47 might be ok

But In hindsight and since u both look at this, the comment at line 73 is not changed by the PR and looks to me like a lingering Déjà vu with the GCC bug?
Should we change or remove that, so that later no one grabs that trap when we change to std fs::?
line 73:
// This can be replaced by boost::filesystem::lexically_relative once boost is bumped to 1.60.

[hebasto]

I think backporting pulls should consider boost 1.47, not this one.

[maflcko]

It might be better if the comment said what version is requried, as opposed to the version that we have right now

[Saibato]

Suggested change

	// account for possible trailing backslash, since we might have only boost 1.47 we could not use fs::relative
	// account for possible trailing backslash, if we would have bumped to boost 1.60 we could use a simple fs::relative call

[hebasto]

Mind looking into alternative #20080?

[Saibato]

Mind looking into alternativ

@hebasto tested #20080 and does pass my simple test bash and in the gui . not sure that fs::canonical is correct for all other instances and all imaginable path and symlinks i can think of where we use getdatadir.so for now i leave this here open as it is boost and make and backwards compatible,

But i also think that stripping in general the '/' is probably the better way but a great mess to test all implications of that.
So might it then be not better to not start the server at all if one try's to use such paths with trailing '/'.

Also if u take charge at this i am fine with this since its probably by its implications more a thing for current core devs than for me.

edit@saibato
tyi; this here strips also any number of '/'

[meshcollider]

Saibato, really appreciate you following through with this PR and contributing a fix! I'm sorry to say though that it appears the consensus is in favour of the approach in #20080 over this one, so I'm going to have to close this. But I appreciate your effort 🎉

[hebasto]

@meshcollider @Saibato #20080 (comment)