Repository: bitcoin/bitcoin (language: cpp)

Issue:
No match for fs::path operator+= when compiling with Boost 1.78
With Boost 1.78 (installed on my system), I get a compiler error:

```
./fs.h: In function ‘fs::path fs::operator+(fs::path, fs::path)’:
./fs.h:91:8: error: no match for ‘operator+=’ (operand types are ‘fs::path’ and ‘std::remove_reference<fs::path&>::type’ {aka ‘fs::path’})
   91 |     p1 += std::move(p2);
      |     ~~~^~~~~~~~~~~~~~~~
```

It appears that in Boost 1.78, `operator+=` became a template function, and our `path` class which subclasses `boost::filesystem::path::path` no longer has an `operator+=` which will work with it.

I also opened an issue with boost: https://github.com/boostorg/filesystem/issues/223


Discussion / hints:
22.x should compile fine, right?
This will be "solved" in master (23.0) by just getting rid of boost:filesystem (#20744).

> 22.x should compile fine, right?

Looks like it does. I've just compiled `22.x` @ 56156a1f08b08127a0d7fc0abd4826f371382a98 on Arch Linux, which packages Boost [1.78.0-1](https://archlinux.org/packages/extra/x86_64/boost/), and that worked fine. Note that for `22.x` depends Boost would also available.

So what we need to decide is what to do for the period between now and when #20744 is merged, for anyone building master, using a system boost >= 1.78.0. Obviously they could also build against depends which is still version 1.71.0.
Boost has committed a fix for this issue, but it won't be available until the next boost release, which is probably going to be in April.
I think the only thing calling code uses this operator+ function for is appending literal ".json" or ".dat" suffixes, so it should be fine to replace it with something less flexible but more portable like:

```diff
diff --git a/src/fs.h b/src/fs.h
index 3cf4371fb4e..f179cc04d5c 100644
--- a/src/fs.h
+++ b/src/fs.h
@@ -85,11 +85,11 @@ static inline auto quoted(const std::string& s)
     return boost::io::quoted(s, '&');
 }
 
-// Allow safe path append operations.
-static inline path operator+(path p1, path p2)
+// Allow adding literal path suffixes, which is safe as long as the suffix is ASCII.
+static inline path operator+(path p, char const* suffix)
 {
-    p1 += std::move(p2);
-    return p1;
+    p += suffix;
+    return p;
 }
 
 /**
```
OpenSuse Tumbleweed is also affected
Just ran into the same problem on my Manjaro (Arch) Linux. With your diff @ryanofsky it compiles but I get test errors:
```
wallet/test/init_tests.cpp(72): error: in "init_tests/walletinit_verify_walletdir_no_trailing": check walletdir == expected_path has failed ["/tmp/test_common_Bitcoin Core/4a5bfaa41ad260303f7dfa2fc8bdf3b9229792fbdb8aadc1853590cf54970ea2/wallets/" != "/tmp/test_common_Bitcoin Core/4a5bfaa41ad260303f7dfa2fc8bdf3b9229792fbdb8aadc1853590cf54970ea2/wallets"]
```

It seems that `fs::canonical` doesn't remove trailing slashes any more. As far as I know this is also not necessary for `std::filesytem::path` (see https://eel.is/c++draft/filesystems#fs.path.generic-6) so it's something that might change in the future.

In any case, this is the diff that works for me (all tests seem to work):

```diff
diff --git a/src/fs.h b/src/fs.h
index 3cf4371fb4..724bb0f79a 100644
--- a/src/fs.h
+++ b/src/fs.h
@@ -88,8 +88,7 @@ static inline auto quoted(const std::string& s)
 // Allow safe path append operations.
 static inline path operator+(path p1, path p2)
 {
-    p1 += std::move(p2);
-    return p1;
+    return p1.concat(p2);
 }
 
 /**
diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp
index 7ef5a0cf55..68e7d001b3 100644
--- a/src/wallet/load.cpp
+++ b/src/wallet/load.cpp
@@ -28,7 +28,7 @@ bool VerifyWallets(WalletContext& context)
         fs::path wallet_dir = fs::PathFromString(args.GetArg("-walletdir", ""));
         boost::system::error_code error;
         // The canonical path cleans the path, preventing >1 Berkeley environment instances for the same directory
-        fs::path canonical_wallet_dir = fs::canonical(wallet_dir, error);
+        fs::path canonical_wallet_dir = fs::canonical(wallet_dir, error).remove_trailing_separator();
         if (error || !fs::exists(wallet_dir)) {
             chain.initError(strprintf(_("Specified -walletdir \"%s\" does not exist"), fs::PathToString(wallet_dir)));
             return false;
 /**
```
`remove_trailing_separator` is undocumented though and doesn't exist for `std::filesystem`.

For anyone who's having issues with Boost 1.78.0, I'm going to beg for review in #20744. Ideally we can fix this issue in master by just removing the Boost filesystem code entirely.
re: https://github.com/bitcoin/bitcoin/issues/23846#issuecomment-1006638894

> Just ran into the same problem on my Manjaro (Arch) Linux. With your diff @ryanofsky it compiles but I get test errors:

Maybe this is tangential if we will use #20744 instead of fixing compability with boost, but it doesn't seem like those test failures have to do the with `operator+` function. They seem like a different compatibility change with fs::canonical, so I don't think there is a problem with my suggested fix for `operator+`.

I do think there is a problem with your suggested fix for `operator+` because the single argument concat method doesn't seem to exist before boost 1.78:

https://www.boost.org/doc/libs/1_77_0/libs/filesystem/doc/reference.html#path-concatenation
https://www.boost.org/doc/libs/1_78_0/libs/filesystem/doc/reference.html#path-concatenation
Temporary workaround for Arch users:
```sh
$ sudo pacman -U https://archive.archlinux.org/packages/b/boost/boost-1.76.0-6-x86_64.pkg.tar.zst https://archive.archlinux.org/packages/b/boost-libs/boost-libs-1.76.0-6-x86_64.pkg.tar.zst
```

Fix patch:
diff --git a/src/fs.h b/src/fs.h
index 9f187945399a1..bc36636084c9b 100644
--- a/src/fs.h
+++ b/src/fs.h
@@ -88,7 +88,7 @@ static inline auto quoted(const std::string& s)
 // Allow safe path append operations.
 static inline path operator+(path p1, path p2)
 {
-    p1 += std::move(p2);
+    p1 += static_cast<boost::filesystem::path&&>(p2);
     return p1;
 }
 
diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp
index 2d47673705019..e6f96074d55ce 100644
--- a/src/wallet/load.cpp
+++ b/src/wallet/load.cpp
@@ -29,7 +29,7 @@ bool VerifyWallets(WalletContext& context)
         fs::path wallet_dir = fs::PathFromString(args.GetArg("-walletdir", ""));
         boost::system::error_code error;
         // The canonical path cleans the path, preventing >1 Berkeley environment instances for the same directory
-        fs::path canonical_wallet_dir = fs::canonical(wallet_dir, error);
+        fs::path canonical_wallet_dir = fs::canonical(wallet_dir, error).remove_trailing_separator();
         if (error || !fs::exists(wallet_dir)) {
             chain.initError(strprintf(_("Specified -walletdir \"%s\" does not exist"), fs::PathToString(wallet_dir)));
             return false;


Test patch:
diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py
index 0b868dde6c43d..317121eb6856c 100755
--- a/test/functional/wallet_multiwallet.py
+++ b/test/functional/wallet_multiwallet.py
@@ -141,7 +141,7 @@ def wallet_file(name):
 
         # should raise rpc error if wallet path can't be created
         err_code = -4 if self.options.descriptors else -1
-        assert_raises_rpc_error(err_code, "boost::filesystem::create_directory:", self.nodes[0].createwallet, "w8/bad")
+        assert_raises_rpc_error(err_code, "boost::filesystem::create_director", self.nodes[0].createwallet, "w8/bad")
 
         # check that all requested wallets were created
         self.stop_node(0)