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

active_epoch_no field in pool_update table is incorrect for updates #610

Closed
Craphtex opened this issue May 11, 2021 · 7 comments
Closed

Comments

@Craphtex
Copy link
Contributor

The field active_epoch_no in pool_update table is always +2 from the epoch of the transaction registering the update, while in reality it should be +2 for registrations and +3 for other updates.

PoC code for all diffs being 2

SELECT
  (active_epoch_no - epoch_no) AS epoch_diff,
  count(*)
FROM pool_update
JOIN tx          ON tx.id    = pool_update.registered_tx_id
JOIN block       ON block.id = tx.block_id
GROUP BY (active_epoch_no - epoch_no);
 epoch_diff | count 
------------+-------
          2 | 16809
(1 row)

Example of pool updates for a pool where there should be a diff of +3 in several places but the diff is recorded as +2

SELECT
  view,
  active_epoch_no,
  epoch_no,
  (active_epoch_no - epoch_no) AS diff
FROM pool_update
JOIN pool_hash   ON pool_hash.id = pool_update.hash_id
JOIN tx          ON tx.id        = pool_update.registered_tx_id
JOIN block       ON block.id     = tx.block_id
WHERE view = 'pool1kjd9ff9u5ytq4atavv2dfsg3pvxqw8d50642pzmpn386zel0ka8'
ORDER BY pool_update.id;
                           view                           | active_epoch_no | epoch_no | diff 
----------------------------------------------------------+-----------------+----------+------
 pool1kjd9ff9u5ytq4atavv2dfsg3pvxqw8d50642pzmpn386zel0ka8 |             210 |      208 |    2
 pool1kjd9ff9u5ytq4atavv2dfsg3pvxqw8d50642pzmpn386zel0ka8 |             210 |      208 |    2
 pool1kjd9ff9u5ytq4atavv2dfsg3pvxqw8d50642pzmpn386zel0ka8 |             212 |      210 |    2
 pool1kjd9ff9u5ytq4atavv2dfsg3pvxqw8d50642pzmpn386zel0ka8 |             221 |      219 |    2
 pool1kjd9ff9u5ytq4atavv2dfsg3pvxqw8d50642pzmpn386zel0ka8 |             223 |      221 |    2
 pool1kjd9ff9u5ytq4atavv2dfsg3pvxqw8d50642pzmpn386zel0ka8 |             235 |      233 |    2
 pool1kjd9ff9u5ytq4atavv2dfsg3pvxqw8d50642pzmpn386zel0ka8 |             257 |      255 |    2
(7 rows)
@MarcelKlammer
Copy link

I also stumbled over that problem today.

I've updated both TITAN pools in the current epoch (e265), active_epoch_no is 267, which is fine for the margin change of one of the pools, but the pledge change will be active next epoch already (e266).

So it should be +1 for pledge changes and +2 for parameter changes, right?

@JaredCorduan
Copy link

All the pool parameters are handled in the same way, the pledge is no different.

@MarcelKlammer
Copy link

Pledge need to be met one epoch prio to be in the snapshot. So we have one epoch transition before we need to meet the newly registered pledge. That's why it looks like a wrong active epoch. As long as it stays consistent, it is fine.

@JaredCorduan
Copy link

It can be a bit hard describing the snapshots. I made this picture recently to try to help: IntersectMBO/cardano-ledger#2282

The pledge (just like cost and margin, and all the pool parameters) is effected by the re-registration logic.

@MarcelKlammer
Copy link

MarcelKlammer commented May 18, 2021

So the "active_epoch_no" is not really "active", but "snapshotted" or "locked_in"

Bildschirmfoto 2021-05-18 um 07 46 23

@erikd
Copy link
Contributor

erikd commented May 18, 2021

The intention of active_epoch_no is for it to indicate that epoch in which the new pool parameters are active.

There is currently an off-by-one bug that will be fixed in the next release.

@MarcelKlammer
Copy link

MarcelKlammer commented May 18, 2021

So, the fix will see the current behavior:

re-register: e265
active_epoch_no: e267

change to

re-register: e265
active_epoch_no: e268

right?

erikd pushed a commit that referenced this issue May 28, 2021
Previously, the value inserted in the activeEpochNo field we calculated
did not match the actual behaviour of the ledger rules.

If the pool is found in `_pParams` of `PState` before the current block it's
`epoch + 3` (retirements in the current block are irrelevant) if it's not
found, but there is already another `PoolReg` in the current block it's
`epoch + 3` again (we can check this from the db). Any other case is
`epoch + 2`.

Closes: #610
erikd pushed a commit that referenced this issue May 28, 2021
Previously, the value inserted in the activeEpochNo field we calculated
did not match the actual behaviour of the ledger rules.

If the pool is found in `_pParams` of `PState` before the current block it's
`epoch + 3` (retirements in the current block are irrelevant) if it's not
found, but there is already another `PoolReg` in the current block it's
`epoch + 3` again (we can check this from the db). Any other case is
`epoch + 2`.

Closes: #610
erikd pushed a commit that referenced this issue May 28, 2021
Previously, the value inserted in the activeEpochNo field we calculated
did not match the actual behaviour of the ledger rules.

The epoch where the update becomes active is the current epoch plus two
or three.

* If the pool is found in `_pParams` field of `PState` before the current
  block its `+3` (retirements in the current block are irrelevant).
* If it's not found, but there is already another `PoolReg` in the current
  block it's `+3` (we can check this from the db).
* Otherwize its `+2`.

Closes: #610
erikd pushed a commit that referenced this issue May 28, 2021
Previously, the value inserted in the activeEpochNo field did not match
the actual behaviour of the ledger rules.

The epoch where the update becomes active is the current epoch plus two
or three.

* If the pool is found in `_pParams` field of `PState` before the current
  block its `+3` (retirements in the current block are irrelevant).
* If it's not found, but there is already another `PoolReg` in the current
  block it's `+3` (we can check this from the db).
* Otherwize its `+2`.

Closes: #610
@erikd erikd closed this as completed in c226c4b May 28, 2021
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

No branches or pull requests

4 participants