Skip to content

/ bitcoin

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

rpc: Remove unused COINBASE_FLAGS #17519

Merged
merged 1 commit into from Nov 22, 2019
Merged

rpc: Remove unused COINBASE_FLAGS #17519

merged 1 commit into from Nov 22, 2019
+2 −9

Conversation

@narula
Copy link
Contributor

narula commented Nov 19, 2019

Commit d449772 stopped setting
COINBASE_FLAGS, and it looks like it hasn't been used since P2SH.

Following up on #17489, remove COINBASE_FLAGS which is unused. I verified that removing this did not change the contents of the coinbase's scriptSig.
@MarcoFalke
MarcoFalke approved these changes Nov 19, 2019
View changes
Copy link
Member

MarcoFalke left a comment

ACK
src/rpc/mining.cpp Show resolved Hide resolved
@luke-jr
luke-jr reviewed Nov 19, 2019
View changes
src/rpc/mining.cpp Show resolved Hide resolved
@MarcoFalke MarcoFalke changed the title refactor: Remove unused COINBASE_FLAGS rpc: Remove unused COINBASE_FLAGS Nov 19, 2019
@MarcoFalke MarcoFalke added RPC/REST/ZMQ and removed Refactoring labels Nov 19, 2019
@laanwj

This comment has been minimized.

Sign in to view
Copy link
Member

laanwj commented Nov 20, 2019 •
edited

ACK 9aedabe
ACK e9a27cf
@narula

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

narula commented Nov 20, 2019

I can update RPCHelpMan. Is the right way to do that just remove flags, or would you prefer it to be marked in some way?

Does it make sense to remove the coinbaseaux flag as well?
@laanwj

This comment has been minimized.

Sign in to view
Copy link
Member

laanwj commented Nov 20, 2019

I can update RPCHelpMan. Is the right way to do that just remove flags, or would you prefer it to be marked in some way?

To remove it, so that the help matches the output.

Does it make sense to remove the coinbaseaux flag as well?

It's part of the BIP according to @luke-jr so I think it'd be better to keep it but leave it empty, as @MarcoFalke says.
@jnewbery

This comment has been minimized.

Sign in to view
Copy link
Member

jnewbery commented Nov 20, 2019

utACK 9aedabe once flags has been removed from the help text.
@narula
refactor: Remove unused COINBASE_FLAGS
Loading status checks…
e9a27cf 
Commit d449772 stopped setting
COINBASE_FLAGS, and it looks like it hasn't been used since P2SH.
Update the help string to remove "flags", which is not specified in
BIP 22.
@narula narula force-pushed the narula:removecoinbaseflags branch from 9aedabe to e9a27cf Nov 21, 2019
@narula

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

narula commented Nov 21, 2019

Updated to remove flags.
@MarcoFalke

This comment has been minimized.

Sign in to view
Copy link
Member

MarcoFalke commented Nov 21, 2019

ACK e9a27cf 💻

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK e9a27cf338dc618b8ecab8984abc54d588de8a05 💻
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhjIQwAsMub4O7fZgenPS9fKJMwQxcxAS+hoZOFtDsGFN2pBFSCgB4oJE5c4u7T
OrTw53bux7AuRuhDjw7oTEp7Gv1J3IIR1dWP0Rnh5Pxjt2cPaoHElTHFWLqoR9tE
n2y7yTsqo2GSTze1JraibYW9CJY7v0ZS2SvrjCfhBR0ybYXKRKnGVFmPuVE2HwTl
jrBZklNRgL9ft/b0azemSXOuI8oy6OsBX56XOaiRGZ/RD7A32QspoePiSPqK1eqP
tHPEvH7BQkeGEVq2pTrdvMkkSbGplIZB3YI7TYzIyEGS2DWa9Kx/ERFCs1NFckP5
axmMc9DIgb+DYfPLx6Rj9YsY0CdQajRX9wmPGNIV04sUcwD19Q3gUE1Q14Ockq3u
9T4es3ppRDqvpXmMhuQMZy899NzLMrgtGx4qLJaFkSNTDZsA2JQdI3lD+uGuCu9D
x1a+ecpAg+UtSXTal3bIu861TgU0npszmJ+3OD37IknfA4vxeHTmPfo5xrYbPOSE
dc28Zx+w
=6gag
-----END PGP SIGNATURE-----

Timestamp of file with hash 4af5f8707e419046118b148a954e705aa3ec85e3d60b38b9f3a0e1e32ec1cb31 -
laanwj added a commit that referenced this pull request Nov 22, 2019
@laanwj
Merge #17519: rpc: Remove unused COINBASE_FLAGS
Verified
This commit was signed with a verified signature.
laanwj Wladimir J. van der Laan
GPG key ID: 1E4AED62986CD25D Learn about signing commits
Loading status checks…
a739d20 
e9a27cf refactor: Remove unused COINBASE_FLAGS (Neha Narula)

Pull request description:

  Commit d449772 stopped setting
  COINBASE_FLAGS, and it looks like it hasn't been used since P2SH.

  Following up on #17489, remove COINBASE_FLAGS which is unused. I verified that removing this did not change the contents of the coinbase's scriptSig.

ACKs for top commit:
  laanwj:
    ACK e9a27cf
  MarcoFalke:
    ACK e9a27cf 💻

Tree-SHA512: f9dac124ce7e3edcae974137764bb5039387b1b123b86af44486e398aa4a8d91a9ecf640e207b364ae303acbbaee7cca300d303ea3d6869ba9cae2bf555a6334
@laanwj laanwj merged commit e9a27cf into bitcoin:master Nov 22, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
sidhujag added a commit to syscoin/syscoin that referenced this pull request Nov 22, 2019
@laanwj @sidhujag
Merge bitcoin#17519: rpc: Remove unused COINBASE_FLAGS
c9cd991 
e9a27cf refactor: Remove unused COINBASE_FLAGS (Neha Narula)

Pull request description:

  Commit d449772 stopped setting
  COINBASE_FLAGS, and it looks like it hasn't been used since P2SH.

  Following up on bitcoin#17489, remove COINBASE_FLAGS which is unused. I verified that removing this did not change the contents of the coinbase's scriptSig.

ACKs for top commit:
  laanwj:
    ACK e9a27cf
  MarcoFalke:
    ACK e9a27cf 💻

Tree-SHA512: f9dac124ce7e3edcae974137764bb5039387b1b123b86af44486e398aa4a8d91a9ecf640e207b364ae303acbbaee7cca300d303ea3d6869ba9cae2bf555a6334
MarkLTZ added a commit to litecoinz-core/litecoinz that referenced this pull request Nov 29, 2019
@MarkLTZ
[Refactor] Remove unused COINBASE_FLAGS (bitcoin#17519)
Verified
This commit was signed with a verified signature.
MarkLTZ Mark LTZ
GPG key ID: ED8E56584D90BB3F Learn about signing commits
cb97b89
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luke-jr luke-jr

@MarcoFalke MarcoFalke

Assignees
No one assigned
Labels
RPC/REST/ZMQ
Projects
None yet
Milestone
No milestone
6 participants
@narula @laanwj @jnewbery @MarcoFalke @luke-jr @fanquake
You can’t perform that action at this time.