ACK. Don't forget to update the help text for the option and add some release notes. A test would be nice.

@MarcoFalke the help text already contains ...setting this too low may abort large transactions.. It feels redundant to add something like If this maximum makes the fee per kB for a transaction lower than -mintxfee, it will be aborted.?

How do I add release notes?

To add release notes run vim doc/release-notes-16192.md and type

Wallet
------

- blabla

Would like to have a test for this too, otherwise ACK.

I think a test must be added.

Concept ACK - please add test

While writing the test I realized that it makes sense to check both the -paytxfee and -mintxfee settings. When capping the fee on -maxtxfee, it should still be above both, imo?

58ac729 is unnecessary as the condition will short-circuit. I.e. GetFee already won't be called if the reason is different.

For the built-in logical AND operator, the result is true if both operands are true. Otherwise, the result is false. This operator is short-circuiting: if the first operand is false, the second operand is not evaluated
https://en.cppreference.com/w/cpp/language/operator_logical

@Empact you're right. Wouldn't it still run the comparison twice in situations where feeCalc.reason != FeeReason::MAXTXFEE

Wouldn't it still run the comparison twice in situations where feeCalc.reason != FeeReason::MAXTXFEE

Would expect it to be optimized out by essentially any compiler.

OK, I can revert it if that's desirable. Any pointers as to why example_test.py is failing?

EDIT: Never mind, it succeeded on the next check-in - must have been some transient issue

Please squash your commits when you've finished addressing review comments.

@fanquake Done!

wallet/test/wallet_tests.cpp:49:25: warning: unused variable 'result' [-Wunused-variable]
    CWallet::ScanResult result = wallet->ScanForWalletTransactions(::ChainActive().Genesis()->GetBlockHash(), {} /* stop_block */, reserver, false /* update */);

Could use:

BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::SUCCESS);

(line numbers are off because I saw this when running the new test case against the current master)

Fixed that in the test. Did however not get that warning myself while compiling (using gcc version 7.4)

FTR I'm running with:

$ gcc --version
Apple LLVM version 10.0.1 (clang-1001.0.46.4)
Target: x86_64-apple-darwin18.6.0

Added a functional test as well.

I think this is ready for a final review

There is some trailing white-space in some files, which is why travis failed.

See also #16257 which removes the fee cap entirely and instead throws if -maxtxfee is exceeded.

@gertjaap appreciate your work! But I think we will close this in favour of #16257 because it seems like it is much safer in general and solves this issue in the process. Thanks for contributing :)