Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set feeCharged with the min fee for core to accept a transaction #2624

Merged
merged 4 commits into from
Jul 25, 2020

Conversation

MonsieurNicolas
Copy link
Contributor

Description

Resolves #2505

replaces #2521

This PR tweaks the behavior when checking validity such that feeCharged in the TransactionResult is always set with the minimum fee that core expects for that transaction to be accepted in the transaction queue and to pass validity in general.

Core was almost doing this, this PR makes it that it's doing it consistently for all instances of txINSUFFICIENT_FEE.

I decided to not touch the code at all in the "apply" case even though I think that with today's logic it's actually not possible to reach some of the conditions that are in the codebase. Given how critical fees are in the system, I figured this was the more conservative approach and that has less chance of breaking in the future.

lib/util/uint128_t.cpp Outdated Show resolved Hide resolved
IMPLICIT_UNSAFE_UINT128_OPS operator uint8_t() const;
IMPLICIT_UNSAFE_UINT128_OPS operator uint16_t() const;
IMPLICIT_UNSAFE_UINT128_OPS operator uint32_t() const;
IMPLICIT_UNSAFE_UINT128_OPS operator uint64_t() const;

// Bitwise Operators
uint128_t operator&(const uint128_t & rhs) const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably shouldn't allow any of these templated operators, because they all violate the principle of "easy to use correctly and hard to use incorrectly". You already did this for the non-member arithmetic operators, but it also applies to (for example) the non-member bitwise operators and the member arithmetic and bitwise operators. This isn't necessarily a complete list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I only changed the ones that could potentially be used in the bug I fixed. We're going to have to take a much blunter instrument for the next round of changes to that class but that's outside of the scope of that PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright I'm fine with that. Let's open an issue for this.

src/herder/TransactionQueue.cpp Outdated Show resolved Hide resolved
src/herder/test/TransactionQueueTests.cpp Outdated Show resolved Hide resolved
src/transactions/FeeBumpTransactionFrame.cpp Outdated Show resolved Hide resolved
src/transactions/TransactionFrame.cpp Show resolved Hide resolved
@jonjove
Copy link
Contributor

jonjove commented Jul 24, 2020

r+ 0e5c2a1

@MonsieurNicolas
Copy link
Contributor Author

had to disable a check in checkApply for protocol versions less than 11: the fee recommendation was not higher in that case because we always take the fee bid in older versions of the protocol

@MonsieurNicolas
Copy link
Contributor Author

I also added a commit to fix the build (doesn't compile when tests are disabled)

@MonsieurNicolas
Copy link
Contributor Author

r+ 8f356f7

@latobarita latobarita merged commit 1648a5c into stellar:master Jul 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Fee-Recommendation to Transaction Submission Response
3 participants