witnet-rust icon indicating copy to clipboard operation
witnet-rust copied to clipboard

feat(node+wallet): estimate transaction priority (aka fees suggestion)

Open aesedepece opened this issue 3 years ago • 49 comments

Tasks

  • [x] Keep track of lowest and highest priorities found in recent epochs (using a circular queue with a fixed capacity)
  • [x] Estimate priority values for different priority tiers (stinky, low, medium, high and opulent)
  • [x] Estimate time-to-block for the same priority tiers
  • [x] Logging current estimation for each epoch when in debug level
  • [x] Add tests
  • [x] Make this feature available through JSONRPC
  • [x] Make this feature available through the CLI
  • [x] Make this feature available through the wallet component
  • [x] Update send, split and join CLI methods for interactive fee choosing with --suggest-fee flag
  • [x] Maybe do the same as above with sendRequest
  • [x] Disallow estimation methods until synced
  • [x] Try to get rid of the big warning below by using some proper fixed point type
  • [x] Move PriorityEngine from ChainState to ChainManager
  • [x] Linting

~~ALL PRIORITY VALUES USED IN THIS FEATURE ARE MULTIPLIED BY 1000 FOR SUB-NANOWIT PRECISION, PROVIDED THAT THE SMALLEST POSSIBLE FEE IS 1 NANOWIT BUT THE LIGHTEST TRANSACTION IS 169 WEIGHT UNITS.~~

~~FOR THE SAKE OF UX, THESE VALUES STILL NEED TO BE PRESENTED TO WALLET USERS IN NANOWIT, IDEALLY WITH 1 DECIMAL DIGIT IF THAT DIGIT IS DIFFERENT FROM 0.~~

fix #2273

aesedepece avatar Aug 31 '22 18:08 aesedepece

[2022-09-01T10:20:15Z DEBUG  witnet_node::actors::chain_manager] The priority engine has received new data. The priority estimate is now:
    ╔══════════════════════════════════════════════════════════╗
    ║ TRANSACTION PRIORITY ESTIMATION REPORT                   ║
    ╠══════════════════════════════════════════════════════════╣
    ║ Data request transactions                                ║
    ╟──────────┬──────────────────┬────────────────────────────║
    ║     Type │ Priority         │ Time-to-block              ║
    ╟──────────┼──────────────────┼────────────────────────────║
    ║   Stinky │ 0                | up to 480 epochs           ║
    ║      Low │ 794              | around 3 epochs            ║
    ║   Medium │ 1053             | around 3 epochs            ║
    ║     High │ 1347             | around 3 epochs            ║
    ║  Opulent │ 6922             | less than 2 epochs         ║
    ╠══════════════════════════════════════════════════════════╣
    ║ Value transfer transactions                              ║
    ╟──────────┬──────────────────┬────────────────────────────║
    ║     Type │ Priority         │ Time-to-block              ║
    ╟──────────┼──────────────────┼────────────────────────────║
    ║   Stinky │ 0                | up to 480 epochs           ║
    ║      Low │ 10               | around 2 epochs            ║
    ║   Medium │ 20               | around 2 epochs            ║
    ║     High │ 30               | around 2 epochs            ║
    ║  Opulent │ 40               | less than 2 epochs         ║
    ╚══════════════════════════════════════════════════════════╝

aesedepece avatar Sep 01 '22 10:09 aesedepece

CLI

Default pretty-print mode:

$ witnet node priority 
╔══════════════════════════════════════════════════════════╗
║ TRANSACTION PRIORITY ESTIMATION REPORT                   ║
╠══════════════════════════════════════════════════════════╣
║ Data request transactions                                ║
╟──────────┬──────────────────┬────────────────────────────║
║     Tier │ Priority         │ Time-to-block              ║
╟──────────┼──────────────────┼────────────────────────────║
║   Stinky │ 0                | up to 496 epochs           ║
║      Low │ 766              | around 4 epochs            ║
║   Medium │ 1005             | around 4 epochs            ║
║     High │ 1275             | around 4 epochs            ║
║  Opulent │ 6922             | less than 2 epochs         ║
╠══════════════════════════════════════════════════════════╣
║ Value transfer transactions                              ║
╟──────────┬──────────────────┬────────────────────────────║
║     Tier │ Priority         │ Time-to-block              ║
╟──────────┼──────────────────┼────────────────────────────║
║   Stinky │ 0                | up to 496 epochs           ║
║      Low │ 6765             | around 2 epochs            ║
║   Medium │ 7959             | around 2 epochs            ║
║     High │ 9153             | around 2 epochs            ║
║  Opulent │ 11000            | less than 2 epochs         ║
╚══════════════════════════════════════════════════════════╝

JSON mode (using --json flag)

$ witnet node priority --json
{"jsonrpc":"2.0","result":{"drt_high":{"priority":1275,"time_to_block":{"Around":4}},"drt_low":{"priority":766,"time_to_block":{"Around":4}},"drt_medium":{"priority":1005,"time_to_block":{"Around":4}},"drt_opulent":{"priority":6922,"time_to_block":{"LessThan":2}},"drt_stinky":{"priority":0,"time_to_block":{"UpTo":496}},"vtt_high":{"priority":9153,"time_to_block":{"Around":2}},"vtt_low":{"priority":6765,"time_to_block":{"Around":2}},"vtt_medium":{"priority":7959,"time_to_block":{"Around":2}},"vtt_opulent":{"priority":11000,"time_to_block":{"LessThan":2}},"vtt_stinky":{"priority":0,"time_to_block":{"UpTo":496}}},"id":"1"}

aesedepece avatar Sep 01 '22 16:09 aesedepece

Wallet API (websockets) support

image

aesedepece avatar Sep 01 '22 16:09 aesedepece

For future reference, this is the JSON response (probably helps when implementing this in a client such as Sheikah, the upcoming light wallet, or the block explorer):

{
   "jsonrpc":"2.0",
   "result":{
      "drt_high":{
         "priority":1275,
         "time_to_block":{
            "Around":4
         }
      },
      "drt_low":{
         "priority":766,
         "time_to_block":{
            "Around":4
         }
      },
      "drt_medium":{
         "priority":1005,
         "time_to_block":{
            "Around":4
         }
      },
      "drt_opulent":{
         "priority":6922,
         "time_to_block":{
            "LessThan":2
         }
      },
      "drt_stinky":{
         "priority":0,
         "time_to_block":{
            "UpTo":496
         }
      },
      "vtt_high":{
         "priority":9153,
         "time_to_block":{
            "Around":2
         }
      },
      "vtt_low":{
         "priority":6765,
         "time_to_block":{
            "Around":2
         }
      },
      "vtt_medium":{
         "priority":7959,
         "time_to_block":{
            "Around":2
         }
      },
      "vtt_opulent":{
         "priority":11000,
         "time_to_block":{
            "LessThan":2
         }
      },
      "vtt_stinky":{
         "priority":0,
         "time_to_block":{
            "UpTo":496
         }
      }
   },
   "id":"1"
}

aesedepece avatar Sep 01 '22 16:09 aesedepece

A couple of remarks / questions:

  1. Is priority here defined as absolute fee / transaction weight? Just wondering because the priorities used as an example are quite high.
  2. It presumably does not make sense to ever show a >4 epochs time for DRT's as they will just time out. Does a stinky category make sense there? Is there an extra "label" defined that just says they likely won't resolve?
  3. For value transfers, note that there is a minimum_vtt_fee_nanowits setting which most likely means that those VTT's will never actually get processed unless there are altruistic miners whom actively set that to 0.

drcpu-github avatar Sep 01 '22 17:09 drcpu-github

1. Is priority here defined as `absolute fee / transaction weight`? Just wondering because the priorities used as an example are quite high.

Please read the big warning above. That explains why they look that high :wink:

2. It presumably does not make sense to ever show a >4 epochs time for DRT's as they will just time out. Does a `stinky` category make sense there? Is there an extra "label" defined that just says they likely won't resolve?

That's not correct. DRTs can stay in the mempool for as long as they want before they enter commitment stage. This is all about time-to-block. Nothing to do with the data request resolution workflow itself.

3. For value transfers, note that there is a [`minimum_vtt_fee_nanowits`](https://github.com/witnet/witnet-rust/blob/2d497aad9e3c8fb27d431378ffa4917c351f5603/witnet.toml#L98) setting which most likely means that those VTT's will never actually get processed unless there are altruistic miners whom actively set that to 0.

In my experience, transactions with 0 fees are mined after a while because, as you suggested, there are still some miners who actively set the minimum to 0. In any case, the minimum priority suggested for the low tier is 10, which for the lightest VTT (169wu) is safely over the minimum.

aesedepece avatar Sep 01 '22 19:09 aesedepece

2. It presumably does not make sense to ever show a >4 epochs time for DRT's as they will just time out. Does a `stinky` category make sense there? Is there an extra "label" defined that just says they likely won't resolve?

Maybe this is a common misconception because we have only hit the DR block space limit seldomly? :thinking:

aesedepece avatar Sep 01 '22 19:09 aesedepece

Please read the big warning above. That explains why they look that high 😉

Oops.

That's not correct. DRTs can stay in the mempool for as long as they want before they enter commitment stage. This is all about time-to-block. Nothing to do with the data request resolution workflow itself.

Interesting, that's news to me. I actually thought that if they were not picked up and resolved within four blocks, they were invalidated and you'd get an InsufficientCommits error.

drcpu-github avatar Sep 01 '22 19:09 drcpu-github

Interesting, that's news to me. I actually thought that if they were not picked up and resolved within four blocks, they were invalidated and you'd get an InsufficientCommits error.

Think about this: before the request enters a block, how would you mark the origin of those 4 epochs limit? Generally speaking, the protocol can only enforce any limitations, penalties, etc. on stuff that it can deterministically derive from the chain state. That's not the case for transactions in the mempool, which doesn't exist from a protocol standpoint :grimacing:

As I recently explained to other devs in the community, this is the same reason why there is no such thing as a "tally fee". The need for a tally (and its content) can be deterministically derived from the chain state. Hence, there's a rule that says that if a block doesn't contain a tally that was supposed to be there, it's invalid. With that rule in place, there's simply no need to use a fee to convince the miner to include the tally :man_shrugging:

aesedepece avatar Sep 01 '22 19:09 aesedepece

Think about this: before the request enters a block, how would you mark the origin of those 4 epochs limit? Generally speaking, the protocol can only enforce any limitations, penalties, etc. on stuff that it can deterministically derive from the chain state. That's not the case for transactions in the mempool, which doesn't exist from a protocol standpoint 😬

Nodes could remove "stale data requests" from their local mempool 4 epochs after they entered if they were not picked, but you are right that this is not a deterministic system.

drcpu-github avatar Sep 01 '22 19:09 drcpu-github

The priority command should always output the estimated time in seconds/minutes instead of epochs, we can't expect users to know if 10 epochs is fast or slow.

tmpolaczyk avatar Sep 06 '22 11:09 tmpolaczyk

I tried to make some unit tests and the estimates are a bit strange, I would expect the fees to always be sorted in order such that stinky < low < medium, etc, but in this case the result is '[1000, 849, 500398, 1149767, 1100000]'


    #[test]
    fn can_estimate_correctly2() {
        // 100 blocks where highest and lowest priorities are 1000000 and 1000
        let priorities = vec![
            Priorities {
                drt_highest: Priority::from_fee_weight(1000000, 1),
                drt_lowest: Some(Priority::from_fee_weight(1000, 1)),
                vtt_highest: Priority::from_fee_weight(1000000, 1),
                vtt_lowest: Some(Priority::from_fee_weight(1000, 1)),
            }; 100
        ];
        let engine = PriorityEngine::from_vec(priorities);
        let estimate = engine.estimate_priority().unwrap();

        let mut vtt_estimates = vec![];
        vtt_estimates.push(estimate.vtt_stinky);
        vtt_estimates.push(estimate.vtt_low);
        vtt_estimates.push(estimate.vtt_medium);
        vtt_estimates.push(estimate.vtt_high);
        vtt_estimates.push(estimate.vtt_opulent);

        let vtt_fees: Vec<_> = vtt_estimates.iter().map(|estimate| estimate.priority.nano_wit).collect();

        panic!("{:?}", vtt_fees);
    }

tmpolaczyk avatar Sep 06 '22 16:09 tmpolaczyk

This is the output I get on mainnet with commit 4fde7cb6c41b2c469d696e31eb3060084197041a

╔══════════════════════════════════════════════════════════╗
║ TRANSACTION PRIORITY ESTIMATION REPORT                   ║
╠══════════════════════════════════════════════════════════╣
║ Data request transactions                                ║
╟──────────┬──────────┬────────────────────────────────────║
║     Tier │ Priority │ Time-to-block                      ║
╟──────────┼──────────┼────────────────────────────────────║
║   Stinky │ 0.000    │ up to 6 hours                      ║
║      Low │ 0.432    │ around 1 minute and 30 seconds     ║
║   Medium │ 0.824    │ around 1 minute and 30 seconds     ║
║     High │ 1.311    │ around 1 minute and 30 seconds     ║
║  Opulent │ 1.590    │ less than 1 minute and 30 seconds  ║
╠══════════════════════════════════════════════════════════╣
║ Value transfer transactions                              ║
╟──────────┬──────────┬────────────────────────────────────║
║     Tier │ Priority │ Time-to-block                      ║
╟──────────┼──────────┼────────────────────────────────────║
║   Stinky │ 0.000    │ up to 6 hours                      ║
║      Low │ 0.100    │ around 1 minute and 30 seconds     ║
║   Medium │ 0.200    │ around 1 minute and 30 seconds     ║
║     High │ 0.300    │ around 1 minute and 30 seconds     ║
║  Opulent │ 0.400    │ less than 1 minute and 30 seconds  ║
╚══════════════════════════════════════════════════════════╝

tmpolaczyk avatar Sep 07 '22 12:09 tmpolaczyk

Also I don't see any way to encode the information that the block still has room for more transactions. Because when blocks are almost empty (1 or 2 transactions per block, but never 0) I would expect the "low" fees to be near 0, and when the blocks are full I would expect "low" to be near the minimum. But the actual estimate is near the minimum in both cases.

tmpolaczyk avatar Sep 07 '22 16:09 tmpolaczyk

I'm wondering if it would be better UX to fix the estimated time to the most common values (for example, 1 minute, 1 hour, 1 day), and then calculate the required fee to get that.

So instead of

0.000    │ up to 6 hours                    
570.089  │ around 1 minute and 30 seconds   
670.693  │ around 1 minute and 30 seconds   
771.297  │ around 1 minute and 30 seconds   
1150.000 │ less than 1 minute and 30 seconds

How about something like:

1 minute   | 570.089
10 minutes |   5.123
1 hour     |   0.000
24 hours   |   0.000

Because with the current approach we currently get some useless estimates like in the example above, because they have the same expected time. Ensuring that the expected time is different does not guarantee that the fees will be different, but maybe it is easier to understand. In the first example my question would be "so what is the lowest priority that gives me around 1 minute 30 seconds?".

tmpolaczyk avatar Sep 08 '22 09:09 tmpolaczyk

Because with the current approach we currently get some useless estimates like in the example above, because they have the same expected time. Ensuring that the expected time is different does not guarantee that the fees will be different, but maybe it is easier to understand. In the first example my question would be "so what is the lowest priority that gives me around 1 minute 30 seconds?".

I like that approach too. Instead of speaking about network conditions, it focuses on the user's expectations, alike to an SLA. Let me prototype this a bit, I think it won't be much of a change.

aesedepece avatar Sep 08 '22 11:09 aesedepece

So, a few concerns after looking at the data from the past 10000 blocks:

  • Value transfer transactions almost never fill up their reserved block space. This means that most of the time a priority of 0 will be a good estimate, because if a block has some empty space then any transaction can be included. This makes it hard to test the case where blocks are full, and that's the interesting case because then the fees are most useful.
  • Data request transactions take up space in the block that includes them, but also in the following blocks until the data request is resolved. This is specified in WIP-0008 as a way to limit the number of concurrent data requests and reduce the maximum load of the network. The consequence is that we can have some blocks that have 0 data requests because the previous block has already filled up all the available data request space. So the fee estimation algorithm needs to handle that case, because a block with no data requests does not mean that a data request with priority 0 could have been included in that block. This happens fairly often in mainnet.

tmpolaczyk avatar Sep 09 '22 13:09 tmpolaczyk

Value transfer transactions almost never fill up their reserved block space. [...] This makes it hard to test the case where blocks are full, and that's the interesting case because then the fees are most useful.

There's a unit test for that case now.

Data request transactions take up space in the block that includes them, but also in the following blocks until the data request is resolved. This is specified in WIP-0008 as a way to limit the number of concurrent data requests and reduce the maximum load of the network. The consequence is that we can have some blocks that have 0 data requests because the previous block has already filled up all the available data request space. So the fee estimation algorithm needs to handle that case, because a block with no data requests does not mean that a data request with priority 0 could have been included in that block. This happens fairly often in mainnet.

Fair point. To be more accurate, data requests take up block space for as long as they stay on the commit stage. If this is causing blocks not to introduce new data request transactions on mainnet, it is only because of a problem with the WIP-0008 spec and the implementation on witnet-rust.

What is this problem? The spec is maybe a bit ambiguous on whether the unsolved weight should be calculated before or after prior to applying the block itself (my bad!).

It is now obvious to me that it should be calculated after, because the ethos of that proposal was setting an upper bound to the number of witnessing acts that can take place over a single epoch. It should be perfectly OK for a block to introduce new data requests as long as it also moves an equally weighted amount of existing ones from the commit to the reveal stages.

However, witnet-rust currently implements the opposite logic: it won't post new data requests at block n if block n-1 already took all the witnessing acts capacity. This unavoidably leads to this silly fact: given a constant influx of data requests, if at any block they are enough to take all the witnessing acts capacity, from that moment on, the chain can only accept data requests once every 2 blocks, provided that no additional commitment rounds are required for any of them. This could be reducing our bandwidth by half :facepalm:

To clarify on the meaning of "calculating the unsolved weight after applying the block", this consists in changing from:

unsolved_weight + added_weight > max_dr_weight

to:

unsolved_weight + added_weight - removed_weight > max_dr_weight

where removed_weight is the weight that the data requests that this block is passing from commit to reveal added in a former block (its own weight).

Despite all of this, the educated guesses that the current priority estimation mechanism provides shouldn't be too far off from reality, because at the end of the day, the priority information in which they are based are indeed the product of the effect described above.

In other words, this won't affect priority but rather add a delay to the time-to-block. Why so? It's of course because we're not keeping track of how long the transaction stood in the mempool waiting in line for spare witnessing acts capacity.

But hey, we do know that the longest time that a "prioritized enough" data request will have to wait is equivalent to the maximum number of commitment rounds (4), hence my claim that the estimations shouldn't be that bad: in the worst case, they're off by 3 minutes, which, arguably, is acceptable for this first approach.

aesedepece avatar Sep 12 '22 14:09 aesedepece

[...] So the fee estimation algorithm needs to handle that case, because a block with no data requests does not mean that a data request with priority 0 could have been included in that block. [...]

The current estimation mechanism does indeed make a difference between:

  1. blocks containing no data request transactions (drt_lowest = None and drt_highest = 0)
  2. blocks containing at least 1 data request transaction that pay 0 fees (drt_lowest = 0 and drt_highest = _)
  3. blocks containing exclusively data request transactions that pay 0 fees (drt_lowest = 0 and drt_highest = 0)

This can be seen in full action in the priority estimation reports produced after the latest changes, which recommend a non-zero stinky fee for data request transactions:

╔══════════════════════════════════════════════════════════╗
║ TRANSACTION PRIORITY ESTIMATION REPORT                   ║
╠══════════════════════════════════════════════════════════╣
║ Data request transactions                                ║
╟──────────┬───────────────┬───────────────────────────────║
║     Tier │ Time-to-block │ Priority                      ║
╟──────────┼───────────────┼───────────────────────────────║
║   Stinky │            6h │ 0.152                         ║
║      Low │            1h │ 1.356                         ║
║   Medium │           15m │ 1.397                         ║
║     High │            5m │ 1.439                         ║
║  Opulent │            1m │ 1.439                         ║
╠══════════════════════════════════════════════════════════╣
║ Value transfer transactions                              ║
╟──────────┬───────────────┬───────────────────────────────║
║     Tier │ Time-to-block │ Priority                      ║
╟──────────┼───────────────┼───────────────────────────────║
║   Stinky │            6h │ 0.100                         ║
║      Low │            1h │ 0.200                         ║
║   Medium │           15m │ 0.300                         ║
║     High │            5m │ 0.400                         ║
║  Opulent │            1m │ 0.500                         ║
╚══════════════════════════════════════════════════════════╝

aesedepece avatar Sep 12 '22 15:09 aesedepece

I tested the case where we alternate one block with transactions and one empty block, to simulate a bit the data request case, and the result is that the estimate is exactly the same as without the empty blocks. Which is good for data requests, this way they won't get stuck, but not sure if value transfer transactions should behave the same way.

This is the code I used:

#[test]
fn can_estimate_in_presence_of_empty_blocks() {
    let mut priorities = vec![
        Priorities {
            drt_highest: Priority::from_fee_weight(1_000_000, 1),
            drt_lowest: Some(Priority::from_fee_weight(1_000, 1)),
            vtt_highest: Priority::from_fee_weight(1_000_000, 1),
            vtt_lowest: Some(Priority::from_fee_weight(1_000, 1)),
        };
        100
    ];

    // Set every block with odd index to empty
    for i in 0..priorities.len() {
        if i % 2 != 0 {
            priorities[i] = Priorities::default();
        }
    }

    let engine = PriorityEngine::from_vec(priorities);
    let estimate = engine.estimate_priority(Duration::from_secs(45)).unwrap();

    let expected = PrioritiesEstimate {
        drt_stinky: PriorityEstimate {
            priority: Priority(OrderedFloat(1000.0)),
            time_to_block: TimeToBlock(21600),
        },
        drt_low: PriorityEstimate {
            priority: Priority(OrderedFloat(1000.0)),
            time_to_block: TimeToBlock(3600),
        },
        drt_medium: PriorityEstimate {
            priority: Priority(OrderedFloat(1000.0)),
            time_to_block: TimeToBlock(900),
        },
        drt_high: PriorityEstimate {
            priority: Priority(OrderedFloat(1000.0)),
            time_to_block: TimeToBlock(300),
        },
        drt_opulent: PriorityEstimate {
            priority: Priority(OrderedFloat(1000.0)),
            time_to_block: TimeToBlock(60),
        },
        vtt_stinky: PriorityEstimate {
            priority: Priority(OrderedFloat(1000.0)),
            time_to_block: TimeToBlock(21600),
        },
        vtt_low: PriorityEstimate {
            priority: Priority(OrderedFloat(1000.0)),
            time_to_block: TimeToBlock(3600),
        },
        vtt_medium: PriorityEstimate {
            priority: Priority(OrderedFloat(1000.0)),
            time_to_block: TimeToBlock(900),
        },
        vtt_high: PriorityEstimate {
            priority: Priority(OrderedFloat(1000.0)),
            time_to_block: TimeToBlock(300),
        },
        vtt_opulent: PriorityEstimate {
            priority: Priority(OrderedFloat(1000.0)),
            time_to_block: TimeToBlock(60),
        },
    };

    assert_eq!(estimate, expected);
}

tmpolaczyk avatar Sep 14 '22 16:09 tmpolaczyk

Not sure if I am doing something wrong but I get some very weird estimates in this one case:

  • Fill the queue with normal priorities (max 1_000_000 min 1_000)
  • Push a single priority with very low min (max 1_000_000 min 1)

And then the next estimate returns 1 as the estimate for all the tiers. I would expect the long-time estimates to not be affected by such outliers.

I also tried to push more normal priorities, and the estimate is exactly the same until the value with min 1 is pushed out of the inner queue, so for 2047 epochs.

Code:

let priorities = vec![
    Priorities {
        drt_highest: Priority::from_fee_weight(1_000_000, 1),
        drt_lowest: Some(Priority::from_fee_weight(1_000, 1)),
        vtt_highest: Priority::from_fee_weight(1_000_000, 1),
        vtt_lowest: Some(Priority::from_fee_weight(1_000, 1)),
    };
    DEFAULT_QUEUE_CAPACITY_EPOCHS
];

let mut engine = PriorityEngine::from_vec(priorities);
let estimate1 = engine.estimate_priority(Duration::from_secs(45)).unwrap();
println!("{}", estimate1);

engine.push_priorities(Priorities {
    drt_highest: Priority::from_fee_weight(1_000_000, 1),
    drt_lowest: Some(Priority::from_fee_weight(1, 1)),
    vtt_highest: Priority::from_fee_weight(1_000_000, 1),
    vtt_lowest: Some(Priority::from_fee_weight(1, 1)),
});

let estimate2 = engine.estimate_priority(Duration::from_secs(45)).unwrap();
println!("{}", estimate2);
assert_eq!(estimate1, estimate2);

tmpolaczyk avatar Sep 15 '22 16:09 tmpolaczyk

And then the next estimate returns 1 as the estimate for all the tiers. I would expect the long-time estimates to not be affected by such outliers.

Nice catch. This is certainly an artifact of how the buckets are mapped back to priority values. The lowest bucket is always mapped to the absolute lowest value. I will improve it.

aesedepece avatar Sep 16 '22 09:09 aesedepece

The capacity issue is not fixed yet, see this test:

#[test]
fn engine_capacity_3() {
    let mut engine = PriorityEngine::with_capacity(3);
    let priorities_list = (1..=9)
        .map(|i| Priorities {
            drt_highest: Priority::from(i),
            drt_lowest: None,
            vtt_highest: Priority::from(i * 2),
            vtt_lowest: None,
        })
        .collect_vec();

    for priorities in &priorities_list {
        engine.push_priorities(priorities.clone())
    }

    assert_eq!(engine.get(0).unwrap(), &priorities_list[8]);
    assert_eq!(engine.get(1).unwrap(), &priorities_list[7]);
    assert_eq!(engine.get(2).unwrap(), &priorities_list[6]);
    assert_eq!(engine.get(3), None);
}

The resulting engine seems to have capacity 2, and engine.get(2).unwrap() panics.

tmpolaczyk avatar Sep 23 '22 13:09 tmpolaczyk

The capacity issue is not fixed yet [...]

Fixed!

aesedepece avatar Sep 23 '22 14:09 aesedepece

The capacity is still broken, now the old test with capacity 2 doesn't pass.

Also I'm thinking about the MINIMUM_TRACKED_EPOCHS variable which is set to 20, because the stinky priority usually provides an estimate that's much greater than 20, so will that be a good enough estimate if we only have data from 20 blocks? Because I guess that for the opulent priority it is enough to look at the previous block to get an accurate estimate, maybe? So my question is, would it make sense to make the MINIMUM_TRACKED_EPOCHS depend on the priority level? So for opulent, set it to 1 epoch, and for stinky set it to the equivalent of 1 day?

tmpolaczyk avatar Sep 26 '22 10:09 tmpolaczyk

The capacity is still broken, now the old test with capacity 2 doesn't pass.

Sorry, I missed your comment clarifying on the issue about the mismatch between our targeted capacity and the rounded up capacity that VecDeque actually uses. This is fixed now, as you suggested, by keeping track of the targeted capacity as a field of PriorityEngine itself. Both cases are tested now.

aesedepece avatar Sep 26 '22 11:09 aesedepece

Also I'm thinking about the MINIMUM_TRACKED_EPOCHS variable which is set to 20, because the stinky priority usually provides an estimate that's much greater than 20, so will that be a good enough estimate if we only have data from 20 blocks?

Under normal circumstances, nodes will have their PriorityEngine full up to their capacity. Only right after a reboot will they be tracking MINIMUM_TRACKED_EPOCHS or any number in between that and the capacity.

As a consequence, as you implied, for nodes tracking a small number of blocks, the estimates for the lowest priority tiers will be obviously off. As the nodes track more and more blocks, the estimates will converge with those provided by nodes that have their PriorityEngines full.

Namely, these biased estimates will only cause users to slightly overpay, but never to underpay. Therefore, even if the estimates look funny for some minutes because the stinky tier will be not that far away from the opulent one, this doesn't pose any threat to UX. My personal take is that we should be fine with this.

[...] would it make sense to make the MINIMUM_TRACKED_EPOCHS depend on the priority level? So for opulent, set it to 1 epoch, and for stinky set it to the equivalent of 1 day?

MINIMUM_TRACKED_EPOCHS is only there to prevent estimating priorities with too small of a sample. This value should be high enough so the sample represents the current state of the chain, but low enough so that a recently synced node can start providing priority estimates in span of time that a human being could reasonably wait for. This parameter is not used when populating or querying the lossy counters, so there's no direct connection between this and the priority tiers.

aesedepece avatar Sep 26 '22 11:09 aesedepece

Alright, this looks merge-able. Shall we test https://github.com/witnet/sheikah/pull/1801 before merging?

tmpolaczyk avatar Sep 27 '22 10:09 tmpolaczyk

Alright, this looks merge-able. Shall we test witnet/sheikah#1801 before merging?

Awesome! I'll try the end-to-end workflow with Sheikah on my side.

aesedepece avatar Sep 27 '22 10:09 aesedepece

So what would be the expected way to use the wallet API now? Something like this?

{"fee":{"relative":"1.0"}}

tmpolaczyk avatar Oct 10 '22 14:10 tmpolaczyk