wallet: Refactor BerkeleyBatch Read, Write, Erase, and Exists functions into non-template functions #19292

This could use documentation. In particular the behavior when pdb is absent is not super intuitive, e.g. it returns false for ReadKey and true for WriteKey.

WriteKey returns true so that it works with dummy database. After #19102, this can (and probably should) be changed to false.

Maybe just early return false here if ret != 0 || datValue.get_data() == nullptr?

Meh. Will change if another push is needed.

This is no longer inside a try-catch block, is that safe?

The try-catch block is for the deserialization operation (the >> operator), not this write.

It's a non-trivial move, so might as well add brackets or make it inline.

Will change if another push is needed.

I found it really confusing that all the const& are replaced by &. Is there any reason for that? If not, it could make sense to fix this up in a follow-up commit.

I agree. If nothing else, const is good documentation about what can potentially be modified and what not.

re: #19292 (review)

I found it really confusing that all the const& are replaced by &. Is there any reason for that? If not, it could make sense to fix this up in a follow-up commit.

I assumed it was necessary to remove const from all the arguments because reading from CDataStream objects changes their internal real read position. This is a reason I suggested replacing all the CDataStream arguments with Spans #18971 (review). This could be done as a later followup.

Both data and size are marked const in the CDataStream class. How would this affect the read position anyway?

I think the reason is that the in-memory key data needs to be purged (in case it is a private key)?

Both data and size are marked const in the CDataStream class. How would this affect the read position anyway?

Guess it wouldn't. I didn't check if these were const because I just remembered CDataStream being goofy about reading. Maybe it's less goofy than I thought but I still think Spans are better.

I agree that spans are better when all that is needed is to pass down data and size. I'll leave this diff here up for grabs:

diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp
index 125bf004e4..46d12546c7 100644
--- a/src/wallet/bdb.cpp
+++ b/src/wallet/bdb.cpp
@@ -804,7 +804,7 @@ std::string BerkeleyDatabaseVersion()
     return DbEnv::version(nullptr, nullptr, nullptr);
 }
 
-bool BerkeleyBatch::ReadKey(CDataStream& key, CDataStream& value)
+bool BerkeleyBatch::ReadKey(Span<char> key, CDataStream& value)
 {
     if (!pdb)
         return false;
@@ -822,7 +822,7 @@ bool BerkeleyBatch::ReadKey(CDataStream& key, CDataStream& value)
     return false;
 }
 
-bool BerkeleyBatch::WriteKey(CDataStream& key, CDataStream& value, bool overwrite)
+bool BerkeleyBatch::WriteKey(Span<char> key, Span<char> value, bool overwrite)
 {
     if (!pdb)
         return true;
@@ -840,7 +840,7 @@ bool BerkeleyBatch::WriteKey(CDataStream& key, CDataStream& value, bool overwrit
     return (ret == 0);
 }
 
-bool BerkeleyBatch::EraseKey(CDataStream& key)
+bool BerkeleyBatch::EraseKey(Span<char> key)
 {
     if (!pdb)
         return false;
@@ -855,7 +855,7 @@ bool BerkeleyBatch::EraseKey(CDataStream& key)
     return (ret == 0 || ret == DB_NOTFOUND);
 }
 
-bool BerkeleyBatch::HasKey(CDataStream& key)
+bool BerkeleyBatch::HasKey(Span<char> key)
 {
     if (!pdb)
         return false;
diff --git a/src/wallet/bdb.h b/src/wallet/bdb.h
index 1e717b95be..f5fa2da76a 100644
--- a/src/wallet/bdb.h
+++ b/src/wallet/bdb.h
@@ -208,10 +208,10 @@ class BerkeleyBatch
     };
 
 private:
-    bool ReadKey(CDataStream& key, CDataStream& value);
-    bool WriteKey(CDataStream& key, CDataStream& value, bool overwrite=true);
-    bool EraseKey(CDataStream& key);
-    bool HasKey(CDataStream& key);
+    bool ReadKey(Span<char> key, CDataStream& value);
+    bool WriteKey(Span<char> key, Span<char> value, bool overwrite = true);
+    bool EraseKey(Span<char> key);
+    bool HasKey(Span<char> key);
 
 protected:
     Db* pdb;

@achow101 Let us know if you want to change this to span or rather have the current version of the pr merged

It looks like my patch only compiles after commit b8740d6737b49229101fbce909ca3917929185b4 (Merge #18468: Span improvements)

I'll leave it for a followup.

+1 on using spans I also thought about making BerkeleyBatch::ReadKey return an Option<CDataStream> instead of passing it in, but dunno.

Done in #19320