app: Use relayFee setting #153
Conversation
JoeGruffins
force-pushed
the
userelayfee
branch
2 times, most recently
from
93d070b to
f2e73f0
Compare
JoeGruffins
force-pushed
the
userelayfee
branch
2 times, most recently
from
6a8caec to
a06b98b
Compare
decred/decred/dcr/account.py
Outdated
| @@ -1907,13 +1907,13 @@ def purchaseTickets(self, qty, price): | |||
| spendLimit=int(round(price * qty * 1.1 * 1e8)), # convert to atoms here | |||
| poolAddress=pi.poolAddress, | |||
| votingAddress=pi.ticketAddress, | |||
| ticketFee=0, # use network default | |||
| ticketFee=DefaultRelayFeePerKb, | |||
There was a problem hiding this comment.
I noticed this is set separately in dcrwallet with setticketfee. We could 1) Leave it like it is. All tickets at the network default 10 atoms/byte rate, or 2) Use the user's relayFee setting, or 3) implement our own setTicketFee.
There was a problem hiding this comment.
When trying to use our relay fee, default and high were fine, but if I tried to use the lower 4 atoms/b setting, dcrdata threw an error back at me. So, either default or the setticketfee you mention. Is default ok for this pr, since that wouldn't change anything, and we can investigate setticketfee in another issue/pr?
|
I can't imagine why we would need the rate as a float. It is unneeded precision. We explicitly convert it int a couple places already. Can we just make it an int from the beginning? |
JoeGruffins
force-pushed
the
userelayfee
branch
from
57b9f7b to
5744d56
Compare
decred/decred/dcr/dcrdata.py
Outdated
| @@ -1125,6 +1143,9 @@ def purchaseTickets(self, keysource, utxosource, req): | |||
| UTXOs. The filterFunc is an optional function to filter UTXOs, | |||
| and is of the form func(UTXO) -> bool. | |||
| req account.TicketRequest: the ticket data. | |||
| relayFee (int): Transaction fees in atoms per kb. | |||
There was a problem hiding this comment.
It's worth mentioning here that for the ticket (but not the split tx), the relayFee is overridden by the req.txFee.
There was a problem hiding this comment.
hmm, why have a relayFee parameter at all? Can we remove one of them?
There was a problem hiding this comment.
Like you discovered, the ticket fee and split tx fee may need different fees. If the user wants to pay the low fee on the split tx, you apparently need to set the ticket separately.
There was a problem hiding this comment.
I see. But what I mean is that we have the split tx fee defined in the req already, so we don't need it again in the constructor, which is overridden by the req.txFee. Maybe I'm not seeing something obvious here? We just need to two fee rates, which are in the req already.
decred/decred/dcr/dcrdata.py
Outdated
| # sendOutputs takes the fee rate in atoms/byte | ||
| splitTx, splitSpent, internalOutputs = self.sendOutputs( | ||
| splitOuts, keysource, utxosource, int(txFeeIncrement / 1000) | ||
| splitOuts, keysource, utxosource, txFeeIncrement // 1000, allowHighFees |
There was a problem hiding this comment.
This seems like it's a bug. sendOutputs doesn't actually take fees in atoms / byte. Shouldn't be coverting here. We've been sending split transactions with low fees, I think.
There was a problem hiding this comment.
Here,s a ticket purchase from a month ago done with tinydecred, but fee looks ok https://testnet.dcrdata.org/tx/0db01ee2735021b979f0a19ca5deca62d1ef615daf63dca0b32be33c62cdabc2/out/32
There was a problem hiding this comment.
Ah, I see. I removed it but, this was being converted back to atoms/ kb inside of sendOutputs.
There was a problem hiding this comment.
You can go to atoms/kB everywhere below the AccountSettingsScreen if you'd like.
There was a problem hiding this comment.
I guess we could go all atoms/byte. I'll make an issue about it later. I think they are all in atoms/kB below the AccountSettingsScreen currently.
|
|
||
| relayFeePerKb = feeRate * 1e3 if feeRate else self.relayFee() | ||
| for (i, txout) in enumerate(outputs): | ||
| checkOutput(txout, relayFeePerKb) | ||
| checkOutput(txout, relayFee) | ||
|
|
There was a problem hiding this comment.
It was converted here. So I guess it did take atoms/b. Should I revert? Or try to make everything atoms/b? Either is fine with me, but some accepting atoms/byte and other atoms/kb is confusing.
Remove the realyFee method from dcrdata and have all relay fees passed in order to separate functions cleanly. Relay fees now depend upon the account calling the dcrdata method.
Because an atom cannot be divided, and we convert it to an integer already in a few places, it should be an integer and not a float from the beginning. Also fix docstrings.
The relay fee is already included in the req, which the caller has defined. They can change the relay fee there.
JoeGruffins
force-pushed
the
userelayfee
branch
from
352e4a7 to
9a686f5
Compare
There was a problem hiding this comment.
This is working well, except now I'm getting rejected when broadcasting a regular transaction at the low fee. Looking at dcrdata output, I see
2020-06-06 18:05:41.663 [ERR] PSQL: SendRawTransaction failed: -1: rejected transaction 59b6d9693deba6fdb84173fd97308fded12b0bc299986641eaf4aadb0c5cd710: transaction 59b6d9693deba6fdb84173fd97308fded12b0bc299986641eaf4aadb0c5cd710 has insufficient priority (2.8568972827586208e+07 <= 5.76e+07)
So the node won't even accept regular transactions at that rate. Could you check if you are seeing the same error, and if so we can readjust.
app: Use relayFee setting.
Make relayFee a field of the blockchain and set that field on initiationand changing.
Remove the realyFee method from dcrdata and have all relay fees passed
in order to separate functions cleanly. Relay fees now depend upon the
account calling the dcrdata method.
Follow up on #152