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

Recently-expired names can't be re-registered #4

Closed
midnightmagic opened this issue Oct 27, 2011 · 10 comments
Closed

Recently-expired names can't be re-registered #4

midnightmagic opened this issue Oct 27, 2011 · 10 comments

Comments

@midnightmagic
Copy link
Contributor

Even though they are reported as expired, it looks like the calculation as to whether to allow expired domains to be re-registered uses the current best-height block and not the origin block.. so domains that are reported as expired hundreds of blocks ago (even if you account for the fact the GetDisplay*() call doesn't calculate it the same way) can't be registered currently.

I suspect the fix is to correct the problem in GetTxOfName and feed the GetExpirationDepth() call the nHeight and not the pindexBest->nHeight, otherwise a whole pile of "expired" names are going into un-re-registerable limbo until we come out of the incremental per-block expiration increase at 24000+12000..

However, I don't know if this was intentional. Nor so I know whether fixing this in my own code will make me generate illegal blocks, so.. meh.

@khalahan
Copy link
Member

The changes has been added in the following commit : a1ccce8

name_firstupdate (line 741 of namecoin.cpp) are refused by the GetTxOfName function (line 456 of namecoin.cpp), which uses pindexBest->nHeight as you said.

Validity of blocks are checked at line 1492, which uses pindexBlock->nHeight too. So, i guess everybody need to change that at the same time to really correct the bug...

@khalahan khalahan reopened this Oct 27, 2011
@midnightmagic
Copy link
Contributor Author

Well, then until we come out of the 12000-block limbo, I guess all those domains are stuck in a netherworld! :-) hah.. The Eternal Grace Period. Speaking of which, we need to find one of those people and find out whether they can name_update their names or not. That would suck even worse..

@khalahan
Copy link
Member

One solution could be to remove GetDisplayExpirationDepth and use only GetExpirationDepth. Then correct pindexBlock->nHeight with nHeight if there is some mistakes.
Result would be that current names displayed as expired would become not expired yet (due to the additional step of GetExpirationDepth before block 48k) as they are currently considered by the network blockchain validation rules.

this would avoid an incompatible changes between old and new clients.

@midnightmagic
Copy link
Contributor Author

I have done this already in my fork; this won't solve the issue because the expires_in calculation passes the correct origin block to the expiration depth function. So, the result of your suggestion is that there's only a few blocks difference between the old expires_in (which is incorrect) and the new expires_in, but the domain itself still technically expires past the 12000 blocks.

As far as I can tell, no domains registered now will exit the expires_in negative limbo until the end of the 12000 block gradual increase we're in. As far as I can tell, the mechanism is broken and in a sort of registration eternal grace period.

@midnightmagic
Copy link
Contributor Author

Is Vince waiting for me to commit a fix and request a pull up..? Is this the only place where discussion of it is going on?

@khalahan
Copy link
Member

This could be solved in two ways :

  • allow registration of names displayed as expired (requires an update of every miner as soon as possible, to avoid waiting too long to register expired names)
  • report the expiration of those names to 12k later (requires an update before block 36k, which could be in about 1 or 2 months)

Which one should we choose ?

@midnightmagic
Copy link
Contributor Author

Effectively we have expiration dates past the end of the gradual 12k scale-up. I think your original inclination is correct: just fix the displayed expiration and just excise the non-functional code. That shouldn't break any compatibility nor cause anything other than make non-upgraded people scratch their heads.

If the patch doesn't exist I'll write it when I get to an actual terminal for a couple hours next.

@khalahan
Copy link
Member

Here is a piece of code : khalahan@1f7b7c2

  • Displayed expires_in values stay the same
  • Validation rules by the network are updated to follow the same rules

Things are simplified with one GetExpirationDepth that always use the block number of last name_operation.

Miners should update before end users if possible, otherwise, people using the patch could register expired domains but their transactions won't be accepted until miners apply the patch.

Please, deeply check the code for mistakes !

@khalahan
Copy link
Member

khalahan commented Nov 1, 2011

A new patch has been writen : khalahan@d82837c

This is another way of fixing the bug :

  • change the display expiration values to what the network really accepts/knows
  • expiration depth is now 36k since we reach block 24k (that was the original intent of vinced' patch)
  • avoid an urgent update of everybody, no incompatible change between old and new version
  • old version shows bad expiration times for names registered after block 12k, the new one corrects this

@khalahan
Copy link
Member

khalahan commented Nov 4, 2011

The issue seems fixed in khalahan@4b7c92d
Will be integrated in trunk soon.

@khalahan khalahan closed this as completed Nov 4, 2011
khalahan pushed a commit that referenced this issue Jul 1, 2013
Reduce sync frequency for blkindex.dat
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

2 participants