[Wallet] Refactoring by using CInputCoin instead of std::pair #10165

Would suggest throwing std::out_of_range if this condition is not true. This should be compatible with all the places this code is called (since they previously would have resulted in out-of-bounds memory accesses).

I agree. That's safer than leaving the object as invalid dummy object, which would be harder to troubleshoot as it's not checked against everywhere..

Would suggest deleting default constructor and IsNull method. Their presence seem to make this class more complex and precarious than is necessary, and could lead to bugs and unintended behavior.

The only place where a nullable CInputCoin seems to be used is in one coinLowestLarger local variable, and you could more easily declare this variable as boost::optional<CInputCoin> coinLowestLarger; (or std::optional in the future) instead of building in a layer of optionality into the type.

I don't think using optional would make the code more clear. I never saw it used anywhere in bitcoin. What about using CInputCoin* for coinLowestLarger instead?

I don't think using optional would make the code more clear.

I think optional's useful. It saves work of having to build optionality into types you define, and more importantly it allows the same type to be optional in some contexts and required in others, which I think is exactly what you want here. std::optional is part of c++17 so eventually it will start becoming more commonly used.

Anyway, this was just a suggestion, so feel free to ignore it if you have a different preference.

What about using CInputCoin* for coinLowestLarger instead?

Using a pointer was actually going to be my first suggestion, but I didn't see a way to make that work because coinLowestLarger is assigned from a local variable. But I think this would be an improvement if it could work.

Prefer to use a typedef (I actually prefer using personally, but some devs are unfamiliar with it and you don't need it as CInputCoin is not templated)

Also, prefer a non-Hungarian name if you're introducing a new class. See #9472 (review) -- Hungarians for new code discouraged.

I'm a bit confused here -- isn't coinLowestLarger always null at this point?

prefer not to have fields intermixed with functions.

Can you invert the logic here if (!P) throw is a bit easier to read.

Might also be good to make it a const CWalletTx& so as to make sure that de-referencability is checked everywhere that this is called.

Ah indeed, C# habits... Just replaced by if(!walletTx).

I would like to do const CWalletTx& in a separate PR, as it need more code change, and I need this PR for another one depending on it.

Ah, I was unclear -- I actually don't mind a == nullptr (although current state is cleaner), I was referring to the throw out_of_range. It's easier if a constructor is: if (! preconditionA) throw A; if (!preconditionB) throw B; ....; rather than if (! preconditionA) throw A; if (preconditionB)...; else throw B;

@JeremyRubin indeed, much clearer. I updated the PR.

Why are these data members public and not private?

because I use them outside the class.

Think about replacing old style "typedef X Y " with the more intuitive C++11 "using Y = X" style

better to do in a separate PR. I want to keep it focused on CInputCoin.

We don't use that style anywhere, introducing it here is not a good idea. I'd prefer to save that for new code.

It is not visible on github but local diff shows me that line endings are weird here:

-using CInputCoin = std::pair<const CWalletTx*, unsigned int>;
+class CInputCoin {^M
+public:^M

^M is 13, so I suppose these are DOS line endings. Looks like it is only in src/wallet/wallet.h, and only the newly introduced code. suggest running:

$ dos2unix src/wallet/wallet.h
dos2unix: converting file src/wallet/wallet.h to Unix format ...

oops. Yes, developping on visual studio... Fixed.