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.
AppInitParameterInteraction() for some other deprecated options such -tor/-socks)
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.
Aren’t the others just using an initwarning, which prints to debug log, but continues?
On Thu, Mar 22, 2018, 11:36 John Newbery notifications@github.com 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 https://github.com/bitcoin/bitcoin/pull/12756#issuecomment-375350688, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmvwgQdTeO5qdUv5IVWkD93dqHTKD-ks5tg8TogaJpZM4S3J-3 .
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.
:+1:
utACK 4757c04.
132@@ -133,8 +133,8 @@ def setup_network(self):
133 which we will use to generate our transactions.
134 """
135 self.add_nodes(3, extra_args=[["-maxorphantx=1000", "-whitelist=127.0.0.1"],
136- ["-blockmaxsize=17000", "-maxorphantx=1000"],
137- ["-blockmaxsize=8000", "-maxorphantx=1000"]])
123@@ -124,7 +124,7 @@ def reorg_test(self):
124 # Reboot node 1 to clear its mempool (hopefully make the invalidate faster)
125 # Lower the block max size so we don't keep mining all our big mempool transactions (from disconnected blocks)
126 self.stop_node(1)
127- self.start_node(1, extra_args=["-maxreceivebuffer=20000","-blockmaxsize=5000", "-checkblocks=5", "-disablesafemode"])
146@@ -147,7 +147,7 @@ def reorg_test(self):
147
148 # Reboot node1 to clear those giant tx's from mempool
149 self.stop_node(1)
150- 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?
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.
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
utACK 4757c04.
Good catch.
