COINBASE_MATURITY constant was added to feature_nulldummy test in #21373. This PR moves the constant to blocktools.py file and uses the constant in more tests as suggested here.
Edit: Goal of this PR is to replace integer constants with COINBASE_MATURITY but not necessarily in all cases because that would mean to read and fully understand all tests. That's out of my time constraints. Any reports where COINBASE_MATURITY should be used are welcome though!
51 | @@ -52,6 +52,9 @@ 52 | # Genesis block time (regtest) 53 | TIME_GENESIS_BLOCK = 1296688602 54 | 55 | +# Coinbase transaction outputs can only be spent after this number of new blocks (network rule)
If you like, there is a definition you can borrow elements from in test/functional/interface_bitcoin_cli.py:
# The block reward of coinbaseoutput.nValue (50) BTC/block matures after
# COINBASE_MATURITY (100) blocks. Therefore, after mining 101 blocks we expect
# node 0 to have a balance of (BLOCKS - COINBASE_MATURITY) * 50 BTC/block.
Thank you for bringing my attention to this file - I like the comment. Anyway, I think the current comment is reasonably good. If you think it can/should be improved, please suggest a better comment and I'll (most likely) accept it :)
51 | @@ -52,6 +52,9 @@ 52 | # Genesis block time (regtest) 53 | TIME_GENESIS_BLOCK = 1296688602 54 | 55 | +# Coinbase transaction outputs can only be spent after this number of new blocks (network rule) 56 | +COINBASE_MATURITY = 100
The commit messages are written with COINBASE_MATURE instead of maturity
You are right. Will fix.
Maybe use a scripted diff for the second commit. I'm not sure you changed all the instances.
Maybe use a scripted diff for the second commit. I'm not sure you changed all the instances.
My understanding of a scripted diff commit feature is that one does his changes, commits the changes using git and modifies the commit message to include a script that corresponds 1:1 with the changes in the commit. If this is correct, then I'm not really sure how to add Python imports in an automated way.
Could you elaborate how would you add the scripted diff here? I have some ideas but none seems too appealing.
More info and examples in https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#scripted-diffs.
Oh, you're right, adding the imports with a scripted diff would be complicated (at least for me it would). Feel free to ignore.
Concept ACK
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
13 | @@ -14,13 +14,12 @@ 14 | """ 15 | import time 16 | 17 | -from test_framework.blocktools import NORMAL_GBT_REQUEST_PARAMS, create_block, create_transaction, add_witness_commitment 18 | +from test_framework.blocktools import COINBASE_MATURITY, NORMAL_GBT_REQUEST_PARAMS, create_block, create_transaction, add_witness_commitment
You could use the chance to change all lines that import the introduced constant to a lexicographically sorted multi-line import, which is now used in most functional tests (to avoid potential merge conflicts):
from test_framework.blocktools import (
COINBASE_MATURITY,
NORMAL_GBT_REQUEST_PARAMS,
add_witness_commitment,
create_block,
create_transaction,
)
(Note also the comma in the last line)
I use VSCode. Can it do this for me? (or possibly PyCharm?) Or is it necessary to do this sorting by hand in general?
Personally I don't have any experience with Python IDEs or code formatting tools (I'm using vim for both C++ and Python code) and also guess most other contributors do it by hand. In this specific PR, as far as I can see there is only a single instance where this is relevant anyway.
I see. I was asking not because of the manual effort but to get some insight how others work to align my workflow. Thanks
I use VSCode. Can it do this for me? (or possibly PyCharm?) Or is it necessary to do this sorting by hand in general?
IIRC black (the python liner) does it automatically for you.
(I believe most contributors are using vim or non-IDEs. Suggest clang-format for the c++ and black/pycodestyle for the python)
29 | @@ -30,6 +30,7 @@ 30 | isn't buried by at least two weeks' work. 31 | """ 32 | 33 | +from test_framework.blocktools import COINBASE_MATURITY 34 | from test_framework.blocktools import ( 35 | create_block,
These two imports should be joined into one.
Done, thank you.
Looks good to me, almost-ACK, left two small nit review comments.
Concept ACK
Concept ACK
A search for 100 and 101 shows the following places that could also be possibly refactored to COINBASE_MATURITY:
I am still very new to the code base and might be wrong about them being related to coinbase maturity.
A search for
100and101shows the following places that could also be possibly refactored toCOINBASE_MATURITY:
Thank you for researching this! I believe you are correct but I need to check those one by one. I will address this in coming days.
13 | +from test_framework.blocktools import ( 14 | + COINBASE_MATURITY, 15 | + create_block, 16 | + NORMAL_GBT_REQUEST_PARAMS, 17 | + add_witness_commitment, 18 | +)
nit: import order
from test_framework.blocktools import (
COINBASE_MATURITY,
NORMAL_GBT_REQUEST_PARAMS,
add_witness_commitment,
create_block,
)
Thank you
ACK bfa9309ad606102f24c9bd3c33dfe78949f09418 🌇