Skip to content

Conversation

S3RK
Copy link
Contributor

@S3RK S3RK commented Jun 29, 2022

Rationale: more accurate non-input fee estimation for txs with >=253 inputs

@laanwj
Copy link
Member

laanwj commented Jun 29, 2022

Concept and code review ACK 25e4762

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 29, 2022

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

Copy link
Member

@furszy furszy left a comment

Choose a reason for hiding this comment

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

Code review ACK 25e4762. left a small nit.

@@ -760,7 +760,8 @@ static std::optional<CreatedTransactionResult> CreateTransactionInternal(

// vouts to the payees
if (!coin_selection_params.m_subtract_fee_outputs) {
coin_selection_params.tx_noinputs_size = 11; // Static vsize overhead + outputs vsize. 4 nVersion, 4 nLocktime, 1 input count, 1 output count, 1 witness overhead (dummy, flag, stack size)
coin_selection_params.tx_noinputs_size = 10; // Static vsize overhead + outputs vsize. 4 nVersion, 4 nLocktime, 1 input count, 1 witness overhead (dummy, flag, stack size)
Copy link
Member

Choose a reason for hiding this comment

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

nit: outputs size is added in the next line now.

Suggested change
coin_selection_params.tx_noinputs_size = 10; // Static vsize overhead + outputs vsize. 4 nVersion, 4 nLocktime, 1 input count, 1 witness overhead (dummy, flag, stack size)
coin_selection_params.tx_noinputs_size = 10; // Static vsize overhead. 4 nVersion, 4 nLocktime, 1 input count, 1 witness overhead (dummy, flag, stack size)

@achow101
Copy link
Member

ACK 25e4762

@achow101 achow101 merged commit 749b80b into bitcoin:master Jun 29, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Jun 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants