wallet: Never schedule MaybeCompactWalletDB when -flushwallet is off #18923

In commit "wallet: Pass unused argsman to StartWallets()" (faccdaba583288273399481b1f7892a910080fda)

Not a big deal and I can fix it later, but this is a bit broken with #10102 because it is trying to pass a whole argsman object from the node to wallet process just to pass a single bool. (See "Interface method parameter and return types should either be serializable or be other interface classes" examples in https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#internal-interface-guidelines)

Recommended fix for this would be to pass -flushwallet bool setting to StartWallets the same way the -wallet list setting gets passed to LoadWallets above: first read in WalletInit::Construct and passed as an argument to MakeWalletClient, stored in the WalletClientImpl instance, and used as needed

Thanks, tried to implement your suggestion in branch 9ddddd2d927b3a752e4698c4771fc68cdc2d9698

Thanks, tried to implement your suggestion in branch 9ddddd2

Branch looks good. Current version is fine too in case you prefer that or the suggestion doesn't work.

I was thinking maybe the best another approach after #19096 would be to add an ArgsManager pointer to the WalletContext struct, and initializing it from a new MakeWalletClient ArgsManager& argument so the wallet filenames and periodic flush arguments and temporary storage could be dropped

add an ArgsManager pointer to the WalletContext struct

thx, done

In commit "test: Avoid overwriting the NodeContext member of the testing setup" (fa00c37cba98794c1c12c4b77e885d831d9bf183)

Wow, good catches. I wonder if there was a compiler warning we could enable that would have caught these.

In commit "wallet: Pass unused args to WalletClientImpl" (fa38755d2fc444589ae506e4edd8597ab25e55e9)

Slight preference for calling this AssertNotNull instead of Ensure just because it could wind up being used widely, and it'd be nice for it to be obvious what it does.

Also for code review purposes, C++ programmers are already pretty trained to know that *ptr is dangerous and can kill the program, while Ensure(ptr) might look more innocuous if it's part of a larger expression. And since the name doesn't contain "Assert" or "Abort", it might actually be clearer if this returned a pointer so the caller is forced to write *. Could also go more generic and provide

template<typename T> T&& Assert(T&& value) { assert(value); return std::forward<T>(value); }

In commit "util: Add Assert identity function" (fa54cf107319609ff44de960b987bd09b46945d0)

Thinking about this, I think it would be better to change signature to T Assert(T&& value) to avoid a possibility of returning a reference to a temporary. Contrived example but I think p would be an invalid reference here returning T&& instead of T:

std::unique_ptr<int>&& p = Assert(MakeUnique<int>(5));

No other change should be needed to fix this other than removing && from return type.

fa73493930e35850e877725167dc9d42e47015c8

nit, if you feel like cleaning it a bit more maybe do this below:

if (env->mapFileUseCount.erase(strFile) == 0) return false

And drop .erase(it).

Thanks, but I'll leave that for a follow-up

I know you'll disagree, but I think it's always better to test the result of an action rather than whether it was logged. In this case, I think you could test the file modification time before and after.

I think both can be done at the same time, but asserting the modification time increases requires a time.sleep(1), since os.path.getmtime returns seconds. I don't like sleeps longer than 200 ms in tests, so I'll leave that for a follow-up ;)

No need for a follow-up. Fast tests are nice too!