1.14.5 fees #2485

rnicoll · 2021-08-22T17:52:26Z

Opening this early so if people are looking at fees, we have a single branch to stage on.

ReverseControl · 2021-08-22T20:33:19Z

@rnicoll The reason fundrawtransaction.py fails is because round_tx_size() is not defined. In particular:

dogecoin/qa/rpc-tests/test_framework/util.py

Lines 519 to 521 in 4c93783

    
           def round_tx_size(tx_size): 
        
               return int(math.ceil(tx_size / 1000.0)) * 1000

does not exist in 1.14.5. I think this is because we no longer round up in kB to calculate fees. I would think removing it and counting actual number of bytes would address that test, but not sure since I am not familiar with the rpc-tests.

EDIT 1: The offending line is 645 in that rpc test.

dogecoin/qa/rpc-tests/fundrawtransaction.py

Line 645 in 81d5fa4

    
           result_fee_rate = result['fee'] * 1000 / round_tx_size(count_bytes(result['hex']))

EDIT 2: I ran it with round_tx_size() removed from 645 and the test passed just fine.

qa/rpc-tests/fundrawtransaction.py

qa/rpc-tests/test_framework/util.py

chalermchai29 · 2021-08-23T05:06:52Z

qa/rpc-tests/test_framework/util.py

@@ -509,15 +509,12 @@ def random_transaction(nodes, amount, min_fee, fee_increment, fee_variants):

 def assert_fee_amount(fee, tx_size, fee_per_kB):
    """Assert the fee was in range"""
-    target_fee = fee_per_kB / 1000
+    target_fee = tx_size * fee_per_kB / 1000
    if fee < target_fee:


if fee < target_fee:
raise AssertionError("Fee of %s DOGE too low! (Should be %s DOGE)"%(str(fee), str(target_fee)))
# allow the wallet's estimation to be at most 2 bytes off
if fee > (tx_size + 2) * fee_per_kB / 1000:
raise AssertionError("Fee of %s DOGE too high! (Should be %s DOGE)"%(str(fee), str(target_fee)))

chalermchai29 · 2021-08-23T05:07:11Z

qa/rpc-tests/test_framework/util.py

    if fee < target_fee:
-        raise AssertionError("Fee of %s BTC too low! (Should be %s BTC)"%(str(fee), str(target_fee)))


raise AssertionError("Fee of %s DOGE too low! (Should be %s DOGE)"%(str(fee), str(target_fee)))
# allow the wallet's estimation to be at most 2 bytes off
if fee > (tx_size + 2) * fee_per_kB / 1000:
raise AssertionError("Fee of %s DOGE too high! (Should be %s DOGE)"%(str(fee), str(target_fee)))

src/amount.cpp

rnicoll · 2021-08-23T19:19:16Z

@ReverseControl Thanks, fixed up fundrawtransaction.py now

rnicoll · 2021-08-23T19:20:01Z

@chalermchai29 I'm confused by a lot of your comments, but I think they're trying to revert back to the old version before we reduced fees?

ReverseControl · 2021-08-23T19:57:54Z

@rnicoll See my comment on their first suggestion. Yes, you are correct. He is working from an old branch.

ReverseControl · 2021-08-23T19:58:59Z

@rnicoll If you could explain to me, offline, how to compute all the fees and how the test framework works I can take care of all these erros.

rnicoll · 2021-08-23T20:03:08Z

@rnicoll If you could explain to me, offline, how to compute all the fees and how the test framework works I can take care of all these erros.

I think I've fixed the errors, next problem is the values in af5e7b6 are very slightly lower than I'd expect. I'd expected:

999.9775
999.775
999.9775

It's likely the transactions are a byte bigger than I've expected, but not sure. If you have the energy to calculate the amounts taking into account transaction actual size, it would be appreciated.

ReverseControl · 2021-08-23T20:05:29Z

@rnicoll Even if for my own reference, I would very much like to understand all the fees. Who pays them, who receives them, when, how, and why. A chart and an explanation would allow me to fly through fixing RPC errors, now and in the future. Or just a chat about them.

rnicoll · 2021-08-23T20:11:21Z

@rnicoll Even if for my own reference, I would very much like to understand all the fees. Who pays them, who receives them, when, how, and why. A chart and an explanation would allow me to fly through fixing RPC errors, now and in the future. Or just a chat about them.

@ReverseControl You've seen #2347 right?

Basics, in the fees from 1.14.5:

Basic transaction fee is 0.01 DOGE * size of the transaction in kilobytes. For example for a typical transaction of 225 bytes, it's 0.225 kB, so the fee is 0.01 * 0.225 = 0.00225 DOGE.
For transactions with an output below the dust threshold (0.01 from 1.14.5 I believe - check the discussion link above for the correct value, and if you can verify the tests cover it properly that'd be great) then a 0.01 DOGE penalty fee is added.
Fees are typically paid by the sender. For example if I send 10 DOGE with a 0.225 kB transaction, I actually input 10.00225 DOGE.
Transaction inputs are outputs from previous transactions, and are consumed entirely when this are used. Anything left over is then sent to a new address as change. The fee is the difference between the inputs and the outputs.
- For example, if the inputs are 11 DOGE, the fee 0.00225 DOGE and the transaction sends 10 DOGE, then 0.99775 (11 DOGE - 10 DOGE - 0.00225 DOGE) sent to a change output. That means the transaction has two outputs, one of 10 DOGE, the other of 0.99775 DOGE.

There's then weird corner cases like relay required fees and block inclusion and free transaction space, but the above covers 99% of cases.

ReverseControl · 2021-08-23T20:17:57Z

@rnicoll Yes, I have read that.

So, questions:

Who pays the relay fees? To whom? And, when?
What is the dust limit? I have seen it quite a bit in the code.
For the last example, who owns the new created wallet to store the change?

The corner cases are the most interesting cases, because those are the ones we have to absolutely get right.

rnicoll · 2021-08-23T20:21:37Z

@ReverseControl Relay fees go to the miners, they're just minimums that nodes use to decide if to relay. The idea is that nodes don't bother to relay anything that wouldn't be mined (because it's just network spam), rather than they're paid to the relaying nodes.

Dust limit is the amount below which an output is considered ridiculously small. Typically any change below dust is just given to the miner as additional fee, because the output isn't worth the data to store it.

The sender owns the change address. There's a much longer write up on change addresses at https://support.blockchain.com/hc/en-us/articles/360018566291-Change-addresses

ReverseControl · 2021-08-23T20:27:23Z

@rnicoll Nodes do not have coins, who pays the relaying fee to the miners?

Good to know what dust limit is and how it is used. I took a look at the link. That makes more sense.

rnicoll · 2021-08-23T20:30:14Z

@rnicoll Nodes do not have coins, who pays the relaying fee to the miners?

Good to know what dust limit is and how it is used. I took a look at the link. That makes more sense.

@ReverseControl Relay fees is just a threshold value, it's not its own fee type. So they're a value which the fee paid by the sender to the miner must exceed, for nodes to be prepared to relay the transaction. That make any sense?

ReverseControl · 2021-08-23T20:41:11Z

@rnicoll I think I understand, so in your first bullet example: the fee paid by the sender is 0.01 * 0.225 KB = 0.00225 DOGE because this value is greater than the Relayfee of .001 DOGEnodes are inclined to relay it to miners, but if a block had size 90 bytes (let's pretend that is possible) 0.01 * 0.09 KB = 0.0009 DOGE then nodes would be inclined to decline sending that transaction to miners?

rnicoll · 2021-08-23T20:45:27Z

@rnicoll I think I understand, so in your first bullet example: the fee paid by the sender is 0.01 * 0.225 KB = 0.00225 DOGE because this value is greater than the Relayfee of .001 DOGEnodes are inclined to relay it to miners, but if a block had size 90 bytes (let's pretend that is possible) 0.01 * 0.09 KB = 0.0009 DOGE then nodes would be inclined to decline sending that transaction to miners?

Very close - relay fee is also in DOGE per kilobyte, so for a 0.225kB tx and a relay fee of 0.001 DOGE per kilobyte, the fee for the transaction must be at least 0.000225 (yes 1/10th of the fee used) for it to be relayed.

So in your example if it was below 0.0009 DOGE it wouldn't be relayed.

ReverseControl · 2021-08-23T20:58:36Z

@rnicoll You mean .00009, right? .001*.09 = .00009, not 0.0009.

So, in essence so long as Basic Transaction > Relay Fee, noted these are both fees per KB, nodes will be inclined to pass your tx to miners?

rnicoll · 2021-08-23T21:10:53Z

@rnicoll You mean .00009, right? .001*.09 = .00009, not 0.0009.

So, in essence so long as Basic Transaction > Relay Fee, noted these are both fees per KB, nodes will be inclined to pass your tx to miners?

Yes to 0.0009, sorry. Basic Transaction >= Relay Fee technically but I think you got the idea

ReverseControl · 2021-08-23T21:30:58Z

@rnicoll Great. For now that is enough. I will ask later about free space and block inclusion fees.

I will take a look af5e7b6 now.

ReverseControl · 2021-08-24T22:55:57Z

@rnicoll @patricklodder Where are all the methods of self.nodes[0] documented? or whatever the index happens to be. I can't find them. I need the list.

ReverseControl · 2021-08-25T13:51:52Z

@rnicoll @patricklodder what is WITNESS_SCALE_FACTOR? What is it used for? Also, what is DEFAULT_BYTES_PER_SIGOP? What is it used for?

patricklodder · 2021-08-25T14:41:27Z

WITNESS_SCALE_FACTOR is inherited from Bitcoin and a control to increase effective blocksize limits without changing actual blocksize limits by introducing Segregated Witness functionality, which is not enabled on Dogecoin. In earlier discussions regarding the soft fork plan it was brought up that this is overly complicated and while it served Bitcoin with an under-the-hood blocksize increase, for Dogecoin this does not make much sense because we have no small/big block debate in the sense of how contentious that was (and still is, kind of) for Bitcoin.

DEFAULT_BYTES_PER_SIGOP is an anti-spam measure that prevents the situation where an attacker could craft small transactions with tons of operations on the verification script, that would cost enormous time to process and basically DoS block verification throughout the network. It limits how many operations are allowed per transaction byte. This was a 2010 CVE: CVE-2010-5138

patricklodder · 2021-08-25T14:44:48Z

Where are all the methods of self.nodes[0] documented?

Those are the RPC methods, you can see them with dogecoin-cli help

rnicoll · 2021-08-25T19:12:35Z

@rnicoll @patricklodder Where are all the methods of self.nodes[0] documented? or whatever the index happens to be. I can't find them. I need the list.

It's basically any of the RPC commands: https://developer.bitcoin.org/reference/rpc/

Practically speaking, best way to find how to call them from Python is to grep for examples elsewhere in the code.

ReverseControl · 2021-08-26T19:24:34Z

@rnicoll I found the ghosting byte. As far as I can tell, this is a nondeterministic error. Long story short, the missing byte disappears in the real signature of a transaction.

For reference:

**********************New Transaction*********************
     Dummy Vin.scripSig Size: 107
Dummy Vin.scriptWitness Size: 16

   Virtual Transaction Size: 226
     Serialized Tx(Dummy) Size: 226
Serialized2 Tx(Empty Sig) Size: 119
            Estimated Fee: 2260000
            Estimated Fee: 2260000
            Estimated Fee: 2260000
 --Transaction Weight(No sig) Size: 476
     --Serialized WTx(No sig) Size: 182
                    SIGNED.
     Real Vin.scripSig Size: 107
Real Vin.scriptWitness Size: 16
 Transaction Weight(Sig) Size: 904
     Serialized WTx(Sig) Size: 289
**********************New Transaction*********************
     Dummy Vin.scripSig Size: 107
Dummy Vin.scriptWitness Size: 16

   Virtual Transaction Size: 226
     Serialized Tx(Dummy) Size: 226
Serialized2 Tx(Empty Sig) Size: 119
            Estimated Fee: 22600000
            Estimated Fee: 22600000
            Estimated Fee: 22600000
 --Transaction Weight(No sig) Size: 476
     --Serialized WTx(No sig) Size: 182
                    SIGNED.
     Real Vin.scripSig Size: 106           <--Expected to be 107.
Real Vin.scriptWitness Size: 16
 Transaction Weight(Sig) Size: 900
     Serialized WTx(Sig) Size: 288
**********************New Transaction*********************
     Dummy Vin.scripSig Size: 107
Dummy Vin.scriptWitness Size: 16

   Virtual Transaction Size: 226
     Serialized Tx(Dummy) Size: 226
Serialized2 Tx(Empty Sig) Size: 119
            Estimated Fee: 2260000
            Estimated Fee: 2260000
            Estimated Fee: 2260000
 --Transaction Weight(No sig) Size: 476
     --Serialized WTx(No sig) Size: 182
                    SIGNED.
     Real Vin.scripSig Size: 107
Real Vin.scriptWitness Size: 16
 Transaction Weight(Sig) Size: 904
     Serialized WTx(Sig) Size: 289
--------------------------------------------------------------
vout 995.00000000
vout 4.97740000
tx1:  226     226
tx2:  225     225
tx3:  226     226
size(bytes) =  226.0    txfee: 0.02260000    change: 4.97740000    txfee + change: 5.00000000    change + sent: 999.9774000
size(bytes) =  225.0    txfee: 0.22500000    change: 4.77400000    txfee + change: 4.99900000    change + sent: 999.7740000
size(bytes) =  226.0    txfee: 0.02260000    change: 4.97740000    txfee + change: 5.00000000    change + sent: 999.9774000

The computed txfee is computed from python and is what the real fee should be. The check txfee + change is expected to be 5. When it is not, it is because the real signature of the transaction returns one byte shorter than the the signature that was used to compute the fee.

https://github.com/rnicoll/dogecoin/blob/1.14.5-fees/src/wallet/wallet.cpp#L2752

The transaction above, after the function call is done executing, is shorter by one byte than the transaction below, the real signing, with some frequency, i.e. this is not deterministic:

https://github.com/rnicoll/dogecoin/blob/1.14.5-fees/src/wallet/wallet.cpp#L2646 .

And this latter is the one used to compute the fee by the wallet, while the former and valid/real-signed one is the one that is stored in the wallet, and the one that Python later retrieves via RPC calls. Which is why, we both compute fees off by one byte on occasion.

Question is: Shouldn't signatures be fixed length all the time under every imaginable circumstance?

@patricklodder @michilumin

Edit 1: Added emphasis where the ghosting byte actually goes ghosting.
Edit 2: Better English above. Also, see ReverseControl@5531445 to run the code that produce the output above.

patricklodder · 2021-08-26T20:59:36Z

Question is: Shouldn't signatures be fixed length all the time under every imaginable circumstance?

No. This is a known "feature", as was also discussed under #2441. It can be prevented by implementing low-R signing, which means you sign in a loop, until you hit an R value that is low enough to fit in the smaller byte array, and then you use that. It is part of the bitcoin baseline used for 1.21, I think.

ReverseControl · 2021-08-26T22:22:07Z

@patricklodder I do not see any discussion in #2441 on this. I am not familiar with these types of signatures. A paper reference would be nice. What would you suggest as a fix for this issue of not having the correct fee value?

rnicoll · 2021-08-27T17:26:49Z

@ReverseControl That makes sense, and I think we're probably okay then.

Aware this may not be all of the changes, but hoping we can start the review cycle at least.

patricklodder · 2021-08-27T20:26:50Z

I do not see any discussion in #2441 on this. I am not familiar with these types of signatures.

#2441 (comment)

See: bitcoin/bitcoin#13666

rnicoll · 2021-08-27T22:46:51Z

@ReverseControl That makes sense, and I think we're probably okay then. I've done some tests and certainly actual tx length and length used when calculating fee aren't necessarily the same, but at least got that nailed down.

ReverseControl · 2021-08-28T22:12:28Z

@rnicoll Though a priority it might not be, given the monetary cost is so minimal, I am rather pedantic about details like this; that is, doing things correctly in the strictest logical sense of the word correct. Would it not be appropriate to fix this issue by first signing the transaction for real and then calculating the fee with the actual transaction that will be recorded in the blockchain? As opposed to making assumptions, calculating the fee, and then signing the transaction that will become the legitimate transaction after the fact.

rnicoll · 2021-08-30T19:03:06Z

@rnicoll Though a priority it might not be, given the monetary cost is so minimal, I am rather pedantic about details like this; that is, doing things correctly in the strictest logical sense of the word correct. Would it not be appropriate to fix this issue by first signing the transaction for real and then calculating the fee with the actual transaction that will be recorded in the blockchain? As opposed to making assumptions, calculating the fee, and then signing the transaction that will become the legitimate transaction after the fact.

That's the issue - we can't know the fee until after signing, and we can't update the fee after signing without breaking the signature. Instead we use a fee value which is long enough to cover the biggest signature we can expect.

* Reduce DEFAULT_FALLBACK_FEE to 1,000,000 Koinu. Note this by itself has no effect as the required fee is higher. * Reduce wallet minimum fees to 0.01 DOGE * Update DEFAULT_DUST_LIMIT * Revise derived values after updating recommended fees * Remove fee rounding from RPC tests * Revert tests back to Bitcoin originals where possible

michilumin

oof. Pretty sure this is good to go. Lots of math to follow but - seems sane.

rnicoll added the do not merge yet label Aug 22, 2021

rnicoll added this to the 1.14.5 milestone Aug 22, 2021

rnicoll added this to In progress in 1.14.5 via automation Aug 22, 2021

rnicoll changed the base branch from master to 1.14.5-dev August 22, 2021 17:54

chalermchai29 suggested changes Aug 23, 2021

View reviewed changes

qa/rpc-tests/fundrawtransaction.py Show resolved Hide resolved

1.14.5 automation moved this from In progress to Review in progress Aug 23, 2021

chalermchai29 previously approved these changes Aug 23, 2021

View reviewed changes

rnicoll dismissed chalermchai29’s stale review via 4d9708a August 23, 2021 19:18

rnicoll force-pushed the 1.14.5-fees branch from c147d40 to 400c4b4 Compare August 25, 2021 18:24

rnicoll marked this pull request as ready for review August 27, 2021 17:25

rnicoll changed the title ~~WIP: 1.14.5 fees~~ 1.14.5 fees Aug 28, 2021

rnicoll force-pushed the 1.14.5-fees branch from e12d3f2 to fc90f01 Compare September 1, 2021 19:43

ReverseControl mentioned this pull request Sep 1, 2021

settxfee no longer working? #1604

Closed

edtubbs approved these changes Sep 3, 2021

View reviewed changes

1.14.5 automation moved this from Review in progress to Reviewer approved Sep 3, 2021

michilumin approved these changes Sep 3, 2021

View reviewed changes

rnicoll merged commit 9c6af6d into dogecoin:1.14.5-dev Sep 3, 2021

1.14.5 automation moved this from Reviewer approved to Done Sep 3, 2021

rnicoll deleted the 1.14.5-fees branch September 3, 2021 09:52

ReverseControl removed the do not merge yet label Sep 6, 2021

patricklodder mentioned this pull request Sep 13, 2021

Hide smart fee UI elements #2566

Closed

patricklodder mentioned this pull request Sep 25, 2021

fees: Tune incremental fee defaults #2590

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1.14.5 fees #2485

1.14.5 fees #2485

rnicoll commented Aug 22, 2021

ReverseControl commented Aug 22, 2021 •

edited

chalermchai29 Aug 23, 2021

chalermchai29 Aug 23, 2021

rnicoll commented Aug 23, 2021

rnicoll commented Aug 23, 2021

ReverseControl commented Aug 23, 2021 •

edited

ReverseControl commented Aug 23, 2021

rnicoll commented Aug 23, 2021

ReverseControl commented Aug 23, 2021

rnicoll commented Aug 23, 2021

ReverseControl commented Aug 23, 2021

rnicoll commented Aug 23, 2021

ReverseControl commented Aug 23, 2021

rnicoll commented Aug 23, 2021

ReverseControl commented Aug 23, 2021

rnicoll commented Aug 23, 2021

ReverseControl commented Aug 23, 2021

rnicoll commented Aug 23, 2021

ReverseControl commented Aug 23, 2021

ReverseControl commented Aug 24, 2021

ReverseControl commented Aug 25, 2021

patricklodder commented Aug 25, 2021

patricklodder commented Aug 25, 2021

rnicoll commented Aug 25, 2021

ReverseControl commented Aug 26, 2021 •

edited

patricklodder commented Aug 26, 2021

ReverseControl commented Aug 26, 2021

rnicoll commented Aug 27, 2021

patricklodder commented Aug 27, 2021

rnicoll commented Aug 27, 2021 •

edited

ReverseControl commented Aug 28, 2021

rnicoll commented Aug 30, 2021

michilumin left a comment

		if fee < target_fee:
		raise AssertionError("Fee of %s BTC too low! (Should be %s BTC)"%(str(fee), str(target_fee)))

1.14.5 fees #2485

1.14.5 fees #2485

Conversation

rnicoll commented Aug 22, 2021

ReverseControl commented Aug 22, 2021 • edited

chalermchai29 Aug 23, 2021

Choose a reason for hiding this comment

chalermchai29 Aug 23, 2021

Choose a reason for hiding this comment

rnicoll commented Aug 23, 2021

rnicoll commented Aug 23, 2021

ReverseControl commented Aug 23, 2021 • edited

ReverseControl commented Aug 23, 2021

rnicoll commented Aug 23, 2021

ReverseControl commented Aug 23, 2021

rnicoll commented Aug 23, 2021

ReverseControl commented Aug 23, 2021

rnicoll commented Aug 23, 2021

ReverseControl commented Aug 23, 2021

rnicoll commented Aug 23, 2021

ReverseControl commented Aug 23, 2021

rnicoll commented Aug 23, 2021

ReverseControl commented Aug 23, 2021

rnicoll commented Aug 23, 2021

ReverseControl commented Aug 23, 2021

ReverseControl commented Aug 24, 2021

ReverseControl commented Aug 25, 2021

patricklodder commented Aug 25, 2021

patricklodder commented Aug 25, 2021

rnicoll commented Aug 25, 2021

ReverseControl commented Aug 26, 2021 • edited

patricklodder commented Aug 26, 2021

ReverseControl commented Aug 26, 2021

rnicoll commented Aug 27, 2021

patricklodder commented Aug 27, 2021

rnicoll commented Aug 27, 2021 • edited

ReverseControl commented Aug 28, 2021

rnicoll commented Aug 30, 2021

michilumin left a comment

Choose a reason for hiding this comment

ReverseControl commented Aug 22, 2021 •

edited

ReverseControl commented Aug 23, 2021 •

edited

ReverseControl commented Aug 26, 2021 •

edited

rnicoll commented Aug 27, 2021 •

edited