Skip to content

Conversation

@scravy
Copy link
Member

@scravy scravy commented Nov 21, 2018

This removed some mining-related RPCs which were introduced in bitcoin for pooled mining but do not make any sense in unit-e. This has been separated from #212.

The RPCs removed are

  • getblocktemplate
  • submitblock
  • getnetworkhashps

The BIPs no longer implemented are

This closes #16

@scravy scravy added the technical debt Cleaning up code which is there for historical reasons label Nov 21, 2018
@scravy scravy self-assigned this Nov 21, 2018
@scravy scravy force-pushed the remove-bip22-bip23 branch 5 times, most recently from 3744445 to 94e42e9 Compare November 21, 2018 14:32
Copy link
Member

@cornelius cornelius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a couple of references to the removed calls in the functional tests. I suppose that this is fixed with another commit?

txmemppol.h contains a reference to getblocktemplate for the nTrascationsUpdated member of the CTxMemPool class. Is this member still needed?

test/README.md mentions submitblock. This can probably just be removed.

utACK

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same nit: s/their/its/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DIdn't you fix this in some commit in this pull request. I thought I saw it changed. Only a very minor thing, though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I had. Maybe a merge issue. Will take care of that in a follow up pull request.

@scravy scravy changed the title remove bip22 and bip23 Remove BIP 22 and BIP 23 Nov 21, 2018
@scravy
Copy link
Member Author

scravy commented Nov 21, 2018

@cornelius That's correct (that there are references to these RPCs). It messes some functional test up pretty good, will check that. Most annoying is submit block and premined blocks, as they use them for testing e.g. segwit O.o Will take care of that.

@Gnappuraz
Copy link
Member

Gnappuraz commented Nov 22, 2018

It would be nice if we could update https://github.com/dtr-org/unit-e-docs/blob/master/bip-removal-reference.md as well. Not sure how many ppl knew about this but I think is important to take not of which BIPs makes it in and which don't.

@cornelius
Copy link
Member

Would it make sense to also document it in https://github.com/dtr-org/unit-e/blob/master/doc/bips.md which BIPs are not implemented?

@Gnappuraz
Copy link
Member

@cornelius sure, the document I posted is more for us not to forget what we did with various BIPs code.

Copy link
Member

@Ruteri Ruteri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK
Travis has some issues with it though, "The job exceeded the maximum log length, and has been terminated."
There are a couple of functional tests that try to use those.

@scravy scravy changed the title Remove BIP 22 and BIP 23 WIP: Remove BIP 22 and BIP 23 Nov 22, 2018
@scravy scravy added the wip Work in progress which is not supposed to be merged yet label Nov 22, 2018
@scravy
Copy link
Member Author

scravy commented Nov 22, 2018

@Ruteri Yeah, I'm ... doing something to some functional tests. Marked it as WIP for now, looking into it.

@Gnappuraz I am aware of that bip removal document, in the PR which this is taken out of I am mentioning that I'm going to update that document. To be frank though I don't like the document, especially since I realized that for example for the BIP 30 removal a removal entry had been added to that document but it had not been removed from the list of actually implemented BIPs in the main repository in the first place. That's a classic double booking and I don't see reason for needing to keep these two in sync. Then again I do not care enough about this and we do not remove BIPs every day (also there's just so many BIPs one could possibly remove).

EDIT: @Gnappuraz see #274

@Gnappuraz
Copy link
Member

@Gnappuraz I am aware of that bip removal document, in the PR which this is taken out of I am mentioning that I'm going to update that document. To be frank though I don't like the document, especially since I realized that for example for the BIP 30 removal a removal entry had been added to that document but it had not been removed from the list of actually implemented BIPs in the main repository in the first place. That's a classic double booking and I don't see reason for needing to keep these two in sync. Then again I do not care enough about this and we do not remove BIPs every day (also there's just so many BIPs one could possibly remove).

I'd be completely fine with removing the document if we make it to reference the commits that change the PR implementation/removal in the BIPs doc. And also the other document is more for helping connecting the dots and see which stuff was removed and how more than being something official, if nobody needs it we can plainly remove it.

@scravy scravy force-pushed the remove-bip22-bip23 branch 3 times, most recently from a7c8b6d to 11bb283 Compare November 22, 2018 14:45
Signed-off-by: Julian Fleischer <julian@thirdhash.com>
@scravy scravy force-pushed the remove-bip22-bip23 branch from 11bb283 to 48a896c Compare November 22, 2018 15:24
@scravy scravy removed the wip Work in progress which is not supposed to be merged yet label Nov 22, 2018
@scravy scravy changed the title WIP: Remove BIP 22 and BIP 23 Remove BIP 22 and BIP 23 Nov 22, 2018
@scravy scravy merged commit 1dfe869 into dtr-org:master Nov 22, 2018
@scravy scravy deleted the remove-bip22-bip23 branch November 27, 2018 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

technical debt Cleaning up code which is there for historical reasons

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove mining/fee related code / rpc commands

5 participants