wallet2_api + smart pointers

[vtnerd]

The wallet2_api uses raw pointers in most situations. In some places, memory leaks exist if an exception is thrown. In other places, the memory management isn't specified in the documentation (in this situation, refresh() will delete all memory returned by getAll()).

So I would like to update everything to std::unique_ptr and std::shared_ptr - but this will break existing code. So my questions are:

• Is the effort worth the temporary compilation breakage pain?
• What downstream systems use wallet2_api.h (outside of the official GUI)?

[0xFFFC0000]

Is the effort worth the temporary compilation breakage pain?

My vote is yes. IMHO it worth it to modernize that part of the code-base.

[SNeedlewoods]

What downstream systems use wallet2_api.h (outside of the official GUI)?

If things work out as proposed here (in step 2), the wallet2_api.h will also be used by the wallet-CLI and wallet-RPC.

[rbrunner7]

@vtnerd, can you elaborate a bit for people like me that don't know the subtleties of C++ memory management, and make a good example of such a memory leakage, how and why it occurs, and how the use of special pointers will prevent it?

[jeffro256]

@rbrunner7 if you allocate an object on the heap, for example using the new operator, this will return a pointer to that object on the heap. Let's say we have a class called CoolObject. To instantiate an instance of this class on the heap, we would write CoolObject *myobj = new CoolObject();. This causes the operating system to allocate memory for that object. If we wanted to free that memory, we would write delete myobj;, which calls the destructor of myobj and then frees the used memory. However, if for some reason we lose the reference to myobj, then that memory can never be released (until we exit the program), which is called a "memory leak". Smart pointers are a form of defensive programming that prevents this. The simplest of which is std::unique_ptr, which is simply a struct that holds a single pointer to an object and calls delete ptr; when the std::unique_ptr goes out of scope. This means that either 1) the object is still in scope or 2) it is deleted, without explicitly having to call a delete expression. This is true even when a C++ exception is thrown, as the unwinding process calls all destructors of objects in stack scope until it hits a catch block or exits the thread.

[rbrunner7]

Thanks for that detailed explanation, @jeffro256. Of course I know that memory leaks are one of the biggest problems of C++ programs, but wasn't aware how easy they can occur. I am still a bit confused now why delete myobj is not part of the standard "object goes out of scope* mechanism, couldn't C++ have been built that way? Anyway, now it's too late of course.

I saw that std::unique_ptr is quite prevalent in the Monero codebase.

In the light of all this I think we should extend them and similar mechanisms to the Wallet API.

[jeffro256]

I am still a bit confused now why delete myobj is not part of the standard "object goes out of scope* mechanism, couldn't C++ have been built that way?

During it's design, C++ needed to be backwards compatible with C, which has raw pointers as well. Also, for object-oriented programming, there needs to be a way to pass references to objects without respect to how they're allocated/stored, or even which exact class that are instantiated from (AKA polymorphism), of which pointers serve that purpose well. Languages like Java do the same thing, where objects are passed by "reference-by-value". However, the Java runtime has a garbage collector which prevents memory leaks, at the cost of CPU overhead to manage that garbage collector. The C++ standards committee is pretty anal about making language features "zero-cost abstractions" when it is possible. Even C++ "references" are basically just syntactic sugar on top of pointers, without any real reference counting.

[vtnerd]

@rbrunner7 to follow up on what @jeffro256 wrote, search for C++ RAII. You should find examples of both pointers and mutexes/locks of the RAII concept in action. FWIW Rust took some of the design concepts, and rolled them into their std. Unfortunately C++ has to support "legacy" code, so raw pointers are still considered valid.