[config] Remove blockmaxsize option #12756

jnewbery · 2018-03-22T14:31:45Z

The blockmaxsize option was marked as deprecated in V0.15.1, and code
was added to convert provided blockmaxsize into blockmaxweight. However,
this code was incorrectly implemented, and blockmaxsize was silently
ignored.

No users have complained about blockmaxsize being ignored, so just
remove it in V0.17.

Fixes #12640

cc @ajtowns

The blockmaxsize option was marked as deprecated in V0.15.1, and code was added to convert provided blockmaxsize into blockmaxweight. However, this code was incorrectly implemented, and blockmaxsize was silently ignored. No users have complained about blockmaxsize being ignored, so just remove it in V0.17.

instagibbs · 2018-03-22T14:45:55Z

concept ACK

practicalswift · 2018-03-22T15:04:29Z

Concept ACK

laanwj · 2018-03-22T15:31:55Z

utACK - should it be an error if this option is provided anyway? (as done in AppInitParameterInteraction() for some other deprecated options such -tor/-socks)

jnewbery · 2018-03-22T15:35:32Z

should it be an error if this option is provided anyway?

Perhaps, although this option has been ignored since V0.15.1. Not sure if we need to start throwing an error now.

Happy to add an error (and test) if people think it's necessary.

laanwj · 2018-03-22T15:36:41Z

Well it maybe that no one noticed because it's silently ignored. But agree it's not critical and I don't particularly care.

maflcko · 2018-03-22T15:37:46Z

Aren't the others just using an initwarning, which prints to debug log, but continues?

…

On Thu, Mar 22, 2018, 11:36 John Newbery ***@***.***> wrote: should it be an error if this option is provided anyway? Perhaps, although this option has been ignored since V0.15.1. Not sure if we need to start throwing an error now. Happy to add an error (and test) if people think it's necessary. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#12756 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGGmvwgQdTeO5qdUv5IVWkD93dqHTKD-ks5tg8TogaJpZM4S3J-3> .

laanwj · 2018-03-22T15:39:08Z

You're right. Some are errors, some are warnings. I think the rationale for making the tor-related ones critical is privacy.

promag · 2018-03-22T15:43:03Z

Throwing an error will force the user to adjust the config once he updates the software - this keeps the config clean and up to date. On the other hand he should read release notes and do the same cleanup..

But agree it's not critical and I don't particularly care.

👍

utACK 4757c04.

achow101 · 2018-03-22T17:06:49Z

utACK 4757c04

sipa · 2018-03-22T23:14:12Z

utACK 4757c04

ajtowns

As noted, I think some of the tests need to be updated to use blockmaxweight rather than just not using blockmaxsize anymore.

Otherwise looks good.

ajtowns · 2018-03-23T02:59:21Z

test/functional/feature_fee_estimation.py

        """
        self.add_nodes(3, extra_args=[["-maxorphantx=1000", "-whitelist=127.0.0.1"],
-                                      ["-blockmaxsize=17000", "-maxorphantx=1000"],
-                                      ["-blockmaxsize=8000", "-maxorphantx=1000"]])


I think these should be changed to -blockmaxweight=68000 and 3200. (The code expects node[2] to not be able to mine all transactions, in particular)

ajtowns · 2018-03-23T03:01:08Z

test/functional/feature_pruning.py

        # Reboot node 1 to clear its mempool (hopefully make the invalidate faster)
        # Lower the block max size so we don't keep mining all our big mempool transactions (from disconnected blocks)
        self.stop_node(1)
-        self.start_node(1, extra_args=["-maxreceivebuffer=20000","-blockmaxsize=5000", "-checkblocks=5", "-disablesafemode"])


Likewise, this should set blockmaxweight=20000 in keeping with the comment above about not mining big mempool transactions.

ajtowns · 2018-03-23T03:06:49Z

test/functional/feature_pruning.py


        # Reboot node1 to clear those giant tx's from mempool
        self.stop_node(1)
-        self.start_node(1, extra_args=["-maxreceivebuffer=20000","-blockmaxsize=5000", "-checkblocks=5", "-disablesafemode"])


This should also probably set blockmaxweight=20000.

The stop_node / start_node here is an attempt to flush the mempool; but I wonder if that actually does any good if the mempool is being persisted to disk anyway?

jnewbery · 2018-03-23T13:35:17Z

I think some of the tests need to be updated

These tests were changed/broken(?) by #11100, when -blockmaxsize was broken. This PR currently doesn't change any behaviour in those tests.

Rather than hold this PR up, can we open an issue to track fixing the miner block sizes in those tests and fix separately?

EDIT: opened issue #12768

hkjn · 2018-03-23T14:17:47Z

utACK 4757c04.

Good catch.

fanquake · 2018-03-26T04:23:59Z

-blockmaxsize needs to be removed from bitcoind.1 & bitcoin-qt.1 in doc/man.

maflcko · 2018-03-26T12:43:23Z

@fanquake Those are only updated before every rc

jnewbery · 2018-03-26T13:11:10Z

Five utACKs (@laanwj @promag @achow101 @sipa @hkjn). Ready for merge?

@ajtowns

4757c04 [config] Remove blockmaxsize option (John Newbery) Pull request description: The blockmaxsize option was marked as deprecated in V0.15.1, and code was added to convert provided blockmaxsize into blockmaxweight. However, this code was incorrectly implemented, and blockmaxsize was silently ignored. No users have complained about blockmaxsize being ignored, so just remove it in V0.17. Fixes #12640 cc @ajtowns Tree-SHA512: 968d71d37bf175c5a02539ddec289a12586f886e1dfe64c1d9aa5e39db48d06d21665153824fac3b11503a55f0812d2f1115a2d726aafd37b76ed629ec0aa671

The blockmaxsize option was marked as deprecated in V0.15.1, and code was added to convert provided blockmaxsize into blockmaxweight. However, this code was incorrectly implemented, and blockmaxsize was silently ignored. No users have complained about blockmaxsize being ignored, so just remove it in V0.17. GitHub-Pull: bitcoin#12756 Rebased-From: 4757c04

8fca086 List support for BIP173 in bips.md (Pieter Wuille) 9645aa6 Remove blockmaxsize option from init.cpp (fanquake) 7847b92 Default to defining endian-conversion DECLs in compat w/o config (Matt Corallo) 1720eb3 qt:Show the entire Window when double clicking on taskbar (Chun Kuan Lee) e055bc0 depends: Fix Qt build with XCode 9.3 (fanquake) 0684cf9 Avoid launching as admin when NSIS installer ends. (JeremyRand) e802c22 [config] Remove blockmaxsize option (John Newbery) f118a7a Fix illegal default `addProxy` and `addrSeparateProxyTor` settings. (251) f60e84d Limit the number of IPs we use from each DNS seeder (e0) Pull request description: Backports: - #12626 Limit the number of IPs addrman learns from each DNS seeder - #12650 gui: Fix issue: "default port not shown correctly in settings dialog" - #12756 [config] Remove blockmaxsize option - #12985 Windows: Avoid launching as admin when NSIS installer ends. - #12946 depends: Fix Qt build with XCode 9.3 - #12998 Default to defining endian-conversion DECLs in compat w/o config - #12999 qt: Show the Window when double clicking the taskbar icon - #13064 List support for BIP173 in bips.md to the 0.16 branch. Tree-SHA512: 3e6b47c54b2cd2bdd81fbc6176cb31e46423f6e05988984d3a09b3535e3cee101ffb071cf753a4beff3c9f0521eb5de4b7c0424a3e97da801d56b4015847ac0f

fanquake · 2018-05-16T14:29:05Z

Backported in #12967

The blockmaxsize option was marked as deprecated in V0.15.1, and code was added to convert provided blockmaxsize into blockmaxweight. However, this code was incorrectly implemented, and blockmaxsize was silently ignored. No users have complained about blockmaxsize being ignored, so just remove it in V0.17. GitHub-Pull: bitcoin#12756 Rebased-From: 4757c04

fanquake added the Mining label Mar 22, 2018

jnewbery mentioned this pull request Mar 22, 2018

Network specific conf sections #11862

Merged

ajtowns reviewed Mar 23, 2018

View reviewed changes

jnewbery mentioned this pull request Mar 23, 2018

[tests] Fix block size config in feature_fee_estimation.py, feature_maxuploadtarget.py and feature_pruning.py #12768

Closed

laanwj merged commit 4757c04 into bitcoin:master Mar 26, 2018

laanwj mentioned this pull request Mar 26, 2018

[Tests] Use blockmaxweight where tests previously had blockmaxsize #12790

Merged

jnewbery deleted the remove_blockmaxsize branch March 26, 2018 13:39

maflcko mentioned this pull request Apr 12, 2018

[0.16] Backports #12967

Merged

yashbhutwala mentioned this pull request Sep 6, 2018

Removing blockmaxsize option from mining as it was removed in 0.16.1 by PR #12756 jlopp/bitcoin-core-config-generator#33

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

[config] Remove blockmaxsize option #12756

[config] Remove blockmaxsize option #12756

Uh oh!

Conversation

jnewbery commented Mar 22, 2018

Uh oh!

instagibbs commented Mar 22, 2018

Uh oh!

practicalswift commented Mar 22, 2018

Uh oh!

laanwj commented Mar 22, 2018

Uh oh!

jnewbery commented Mar 22, 2018

Uh oh!

laanwj commented Mar 22, 2018

Uh oh!

maflcko commented Mar 22, 2018 via email

Uh oh!

laanwj commented Mar 22, 2018

Uh oh!

promag commented Mar 22, 2018

Uh oh!

achow101 commented Mar 22, 2018

Uh oh!

sipa commented Mar 22, 2018

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

ajtowns Mar 23, 2018

Choose a reason for hiding this comment

Uh oh!

ajtowns Mar 23, 2018

Choose a reason for hiding this comment

Uh oh!

ajtowns Mar 23, 2018

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hkjn commented Mar 23, 2018

Uh oh!

fanquake commented Mar 26, 2018

Uh oh!

maflcko commented Mar 26, 2018

Uh oh!

jnewbery commented Mar 26, 2018

Uh oh!

fanquake commented May 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

jnewbery commented Mar 23, 2018 •

edited

Loading