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.

Sign up for GitHub

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

Jump to bottom

util: remove unwanted fields from bitcoin-cli -getinfo #17650

Merged
merged 1 commit into from Dec 4, 2019
Merged

util: remove unwanted fields from bitcoin-cli -getinfo #17650

merged 1 commit into from Dec 4, 2019

Conversation

malevolent
Copy link
Contributor

@malevolent malevolent commented Dec 2, 2019 •
edited by fanquake

Removed the following fields from -getinfo: protocolversion, walletversion and keypoololdest.

@malevolent malevolent changed the title Removed unwanted fields from -getinfo. Closes #17314 Removed unwanted fields from -getinfo. Dec 2, 2019
@laanwj
Copy link
Member

laanwj commented Dec 2, 2019

Concept ACK

Code changes look good to me. Some git nits:

  • Please adapt your commit message to the standard format: a short title, an empty line then the body of the text
  • Please set a username and mail address other than "EC2 Default User"

@practicalswift
Copy link
Contributor

Concept ACK

@malevolent Welcome as a contributor! :)

@emilengler
Copy link
Contributor

emilengler commented Dec 2, 2019 •
edited

Concept ACK

Your tests are failing.
Please update test/functional/interface_bitcoin_cli.py
See line 61-76

@malevolent
Copy link
Contributor Author

@laanwj Yes sir, done and done.
@practicalswift Thank you :)
@emilengler Thanks, just pushed change to delete tests for removed api calls.

@fanquake fanquake changed the title Removed unwanted fields from -getinfo. util: remove unwanted fields from bitcoin-cli -getinfo Dec 3, 2019
@fanquake
Copy link
Member

fanquake commented Dec 3, 2019

Please squash your commits.

Please adapt your commit message to the standard format: a short title, an empty line then the body of the text

You could use a commit title like util: remove unwanted fields from bitcoin-cli -getinfo.

@malevolent
Copy link
Contributor Author

Hi @fanquake , thanks for letting me know, I rebased and used better titles for the commit messages. Please let me know if theres anything else needed

@practicalswift
Copy link
Contributor

ACK 868a322 -- diff looks correct

@laanwj
Copy link
Member

laanwj commented Dec 3, 2019

the author is still reported as "EC2 Default User"

commit 868a32228688afca262f10d3be9fa6caa9cdea65
Author: EC2 Default User <ec2-user@ip-172-31-6-106.us-west-1.compute.internal>

i think you need to do git commit --amend --reset-author to fix this, if you want to be credited correctly

@malevolent
util: remove unwanted fields from bitcoin-cli -getinfo
01c8701 
In accordance with bitcoin#17314, Removing noisy fields from -getinfo. Fields removed: protocolversion, walletversion and keypoololdest. In addition to changing bitcoin-cli -getinfo, there is another change to test/functional/interface_bitcoin_cli.py. This change deletes tests that utilize removed -getinfo calls.
@malevolent
Copy link
Contributor Author

@laanwj Thanks I think that fixed it, let me know if there is anything else I need to do

@laanwj
Copy link
Member

laanwj commented Dec 4, 2019

ACK 01c8701

@laanwj laanwj mentioned this pull request Dec 4, 2019
Closed
@practicalswift
Copy link
Contributor

ACK 01c8701 -- diff looks correct

@malevolent
Copy link
Contributor Author

@laanwj is there anything I need to do for the release note label that was added? Also, I think I should change it from saying "closes #17314" since there are other parts to that issue, right?

@fanquake
Copy link
Member

fanquake commented Dec 4, 2019

release note label that was added?

This can be addressed at a later date.

I think I should change itfrom saying "closes #17314" since there are other parts to that issue, right?

Correct, that issue doesn't need to be closed after this is merged.

@achow101
Copy link
Member

achow101 commented Dec 4, 2019

ACK 01c8701

fanquake added a commit that referenced this pull request Dec 4, 2019
@fanquake
Merge #17650: util: remove unwanted fields from bitcoin-cli -getinfo
5aee0e2 
01c8701 util: remove unwanted fields from bitcoin-cli -getinfo (malevolent)

Pull request description:

  Removed the following fields from -getinfo: protocolversion, walletversion and keypoololdest. This change closes #17314 .

ACKs for top commit:
  laanwj:
    ACK 01c8701
  achow101:
    ACK 01c8701
  practicalswift:
    ACK 01c8701 -- diff looks correct

Tree-SHA512: c98f2e8a3fee04d46766f70cb88f4e49e892a4424eff8940a7d48e9e808597b702427225788f984f5c3641591fd8d86acee56774afde1d57a4259c31d971ea08
@fanquake fanquake merged commit 01c8701 into bitcoin:master Dec 4, 2019
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 4, 2019
@fanquake
Merge bitcoin#17650: util: remove unwanted fields from bitcoin-cli -g…
4615472 
…etinfo

01c8701 util: remove unwanted fields from bitcoin-cli -getinfo (malevolent)

Pull request description:

  Removed the following fields from -getinfo: protocolversion, walletversion and keypoololdest. This change closes bitcoin#17314 .

ACKs for top commit:
  laanwj:
    ACK 01c8701
  achow101:
    ACK 01c8701
  practicalswift:
    ACK 01c8701 -- diff looks correct

Tree-SHA512: c98f2e8a3fee04d46766f70cb88f4e49e892a4424eff8940a7d48e9e808597b702427225788f984f5c3641591fd8d86acee56774afde1d57a4259c31d971ea08
@laanwj
Copy link
Member

laanwj commented Dec 5, 2019 •
edited

release note label that was added?

Yea, it's probably better to wait until the release in this case, if there's other bitcoin-cli changes at well they should be grouped and not scattered over the place.

@MarkLTZ MarkLTZ mentioned this pull request Apr 4, 2020
Closed
@fanquake
Copy link
Member

@laanwj Could you add any relevant release notes to https://github.com/bitcoin-core/bitcoin-devwiki/wiki/0.20.0-Release-Notes-Draft ?

@maflcko
Copy link
Member

maflcko commented May 23, 2020

Done https://github.com/bitcoin-core/bitcoin-devwiki/wiki/0.20.0-Release-Notes-Draft/_compare/b03541ec204bd2c7460b3c383c45f84b21c5b65c...3f531649bb00d9824814ff3a7e9c7282e806844b

deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Nov 6, 2020
@malevolent @PiRK
util: remove unwanted fields from bitcoin-cli -getinfo
e58b649 
Summary:
> In accordance with #17314, Removing noisy fields from -getinfo. Fields removed: protocolversion, walletversion and keypoololdest. In addition to changing bitcoin-cli -getinfo, there is another change to test/functional/interface_bitcoin_cli.py. This change deletes tests that utilize removed -getinfo calls.

Add a release note about this and also about new fields introduced in D8220/[[bitcoin/bitcoin#17302 | PR17302]]. This is extracted from Core `release-notes-0.20.0.md`

This is a backport of Core [[bitcoin/bitcoin#17650 | PR17650]]

Test Plan:
`src/bitcoin-cli -getinfo`

`test/functional/test_runner.py interface_bitcoin_cli.py`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D8300
sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
@fanquake
Merge bitcoin#17650: util: remove unwanted fields from bitcoin-cli -g…
0ca1b02 
…etinfo

01c8701 util: remove unwanted fields from bitcoin-cli -getinfo (malevolent)

Pull request description:

  Removed the following fields from -getinfo: protocolversion, walletversion and keypoololdest. This change closes bitcoin#17314 .

ACKs for top commit:
  laanwj:
    ACK 01c8701
  achow101:
    ACK 01c8701
  practicalswift:
    ACK 01c8701 -- diff looks correct

Tree-SHA512: c98f2e8a3fee04d46766f70cb88f4e49e892a4424eff8940a7d48e9e808597b702427225788f984f5c3641591fd8d86acee56774afde1d57a4259c31d971ea08
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
Utils/log/libs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@malevolent @laanwj @practicalswift @emilengler @fanquake @achow101 @maflcko