19.0 code review #201
19.0 code review #201
Conversation
Add GetPowHash, nHeight, IsWitnessEnabled CheckBlockHeader needs g_chainman for LookupBlockIndex
Update contrib/debian/copyright
ASN filters out up to half of the nodes while VTC does not have many to begin with See PR #175
vtc: fixes startup issue relating to verthash generation Don't schedule class PeerManagerImpl's background tasks from its constructor, but instead do that from a separate method, StartScheduledTasks(), that can be called later at the end of startup, after other things, such as the active chain, are initialzed.
Add PGP Fingerprint for Matthew Cummings
|
Been testing this for a while. So far, all the task I do with vertcoind/vertcoin-cli works fine. I'm creating block fine while mining and I can list/explore transaction/mempool. +1 from me |
There was a problem hiding this comment.
In prior release but not in new release (non-critical)
contrib/devtools/gen-manpages.sh # Can we generate Vertcoin branded manpages?
contrib/seeds/makeseeds.py # Slight differences between v0.18 & v19
src/test/netbase_tests.cpp # Looks like the ports were not updated for Vertcoin
src/qt/bitcoinstrings.cpp # *vertcoin_strings[] vs *bittcoin_strings[] - 0.18 has it as *bittcoin_strings[] - also could use a QT_TRANSLATE_NOOP("vertcoin-core", "Vertcoin Core") in the file
src/Makefile.am # How come bignum.h was removed? Line 700, (vertcoin_bin_ldadd) should be used as it was in v0.18?
Vertcoin rebranding strings
doc/man/ # Since we're renaming all the manpage files to 'vertcoin-X.1', should we also update the contents to be vertcoin branded?
src/qt/guiutil.cpp # Vertcoin Core 0.18 changed bitcoin to vertcoin in comments
src/qt/intro.cpp # Vertcoin Core 0.18 changed bitcoin to vertcoin in comments
src/qt/sendcoinsdialog.cpp # Missed some Vertcoin branding and PSVT vs PSBT crops up again
src/qt/forms/receiverequestdialog.ui # Missed an update for vtc1 and VTC string
src/qt/bitcoingui.cpp # PSBT vs PSVT - 0.18 has it PSVT (Partially Sent Vertcoin Transaction)
src/rpc/misc.cpp # Vertcoin Core 0.18 changed bitcoin to vertcoin in comments
src/wallet/test/wallet_tests.cpp # Update strings from bitcoin to vertcoin
src/bitcoin-cli.cpp # Update for a number of strings from bitcoin to vertcoin
src/bitcoind.cpp # Update for a number of strings from bitcoin to vertcoin
Vertcoin not updated in comments (but was updated in prior release)
src/util/system.cpp # Vertcoin Core 0.18 changed bitcoin to vertcoin in comments. How is the CopyrightHolders function working? Why the strPrefix2?
src/policy/fees.cpp # only a difference in a comment. BTC-per-kb vs VTC-per-kb
src/Makefile.qt.include # Vertcoin Core 0.18 changed bitcoin to vertcoin in comments
General questions
src/util/system.h # How is the CopyrightHolders function working? Why the strPrefix2?
src/crypto/verthash.cpp # How did you know to use gArgs.GetDataDirNet() instead of GetDataDir() (0.18 uses GetDataDir())
src/crypto/verthash_datafile.cpp # How did you know to use gArgs.GetDataDirNet() instead of GetDataDir() (0.18 uses GetDataDir())
contrib/seeds/nodes_test.txt # can we provide a list of known testnet seeds? or remove this file?
contrib/verifybinaries/ # Should we remove this directory from Vertcoin?
src/bignum.h # is this part of KimotoGravityWell? It is used by pow.cpp
src/chainparamsseeds.h # How come the seeds are not needed for Vertcoin in BIP155 format?
src/pow.cpp # How did you know to fix the "blockstogoback" logic (line 87-94)
configure.ac # how come we're no longer enabling the SSE2 compiler extension?
src/chainparams.cpp # Should we set nPowTargetTimespan to 3.5 days for SigNet and RegTest?
src/chainparams.cpp # Should we set 2.5 min nPowTargetSpacing for SigNet adn RegTest?
Scheduler related questions
src/test/util/setup_common.cpp # Why was *m_node.scheduler, removed from line 202?
src/test/denialofservice_tests.cpp # Why was *m_node.scheduler removed from peerman call?
src/init.cpp # Why was *m_node.scheduler removed from peerman call? Explain to me how the node peer scheduler works?
Sure, I don't see why not.
Old comment is removed https://github.com/vertcoin-project/vertcoin-core/blob/0.18.0/contrib/seeds/makeseeds.py#L30
Okay, will update these.
bignum.h is included https://github.com/vertcoin-project/vertcoin-core/blob/19.0/src/Makefile.am#L124 |
Will be covered when we generate Vertcoin branded manpages.
I think we can leave comments and anything that is not user facing.
Got it. As for PSBT, I'm opposed to adjusting the
Check comment above referring to global recognition of acronym 'PSBT'
Let's leave comments alone.
Got it.
Got it. Leaving comments alone. |
The strPrefix2 is to allow two copyright holders (Vertcoin Developers and Bitcoin Developers) to appear in places like the opening splash screen.
Leaving comments alone. |
There was a compiling error without and I adjusted to how upstream manages it now.
Making a list of testnet seeds would be a good idea. @cruelnovo could generate such a list from their seeder.
Looks like this hasn't been updated post-guix on upstream and is awaiting to be refactored or removed. It doesn't work anyway but let's leave it. We will instead make the verification process as clear as possible in each release.
Yes, 79e8a0b
That's just how the contrib/seeds/generate-seeds.py script works now.
This was in Vertcoin historically and is a remnant from Litecoin litecoin-project/litecoin@b1b31d1
It was never enabled by default and has been broken until 0.18.0. It's also unclear if it works properly given there is no test written for it. Scrypt-N blocks verify quickly so I don't see it as a huge issue. This can always be looked at and added later by someone interested. Related issue is #177
That would be a target spacing for KGW of which only mainnet uses. |
Related commit: 1ababdd Without this commit, startup fails when having to create verthash. I'm not sure as to why but I know it has something to do with the order in which verthash is generated in src/init.cpp. This commit is backported from upstream 23.0. |
* rpc: Capture potentially large UniValue by ref for rpcdoccheck Github-Pull: 25237 Rebased-From: 20ff499 * Add pure Python RIPEMD-160 Github-Pull: 23716 Rebased-From: ad3e9e1 * Swap out hashlib.ripemd160 for own implementation Github-Pull: 23716 Rebased-From: 5b559dc * windeploy: Renewed windows code signing certificate Github-Pull: #25201 Rebased-From: 7e9fe6d * Prevent data race for `pathHandlers` Github-Pull: bitcoin/bitcoin#25983 Rebased-From: 4296dde * Revert "build: Use Homebrew's sqlite package if it is available" This reverts ee7b84e from #20527. This change was made without any rationale, maybe other than a brew installed version might be newer, and that's "better". However when building from source on macOS, it just results in drastically worse perofrmance, and results in issues / confusions like #25724. Resolves the "build from source" portion of #25724. Building from depends is still not ideal, however I have some other changes that might help improve things in that case. The difference in performance can be observed using the example from bitcoin/bitcoin#25724 (comment), but minified to only 10 descriptors. i.e: ```bash time src/bitcoin-cli createwallet speedy true time src/bitcoin-cli importdescriptors '[ {"desc":"raw(00145846369f3d6ba366d6f5a903fb5cf4dca3763c0e)#k9wh6v62","timestamp":"now"}, {"desc":"raw(001420800aabf13f3a4c4ce3ce4c66cecf1d17f21a6e)#6m0hlfh4","timestamp":"now"}, {"desc":"raw(0014c6bf9715e06d73ebf9b3b02d5cc48d24d8bbabc1)#wyavh36r","timestamp":"now"}, {"desc":"raw(00141ba7807b3f46af113beaea5c698428ce7138cd8a)#jctdsups","timestamp":"now"}, {"desc":"raw(00140c1bd27f10fff01b36ddf3c1febaa1acff19b080)#9s6nc3pk","timestamp":"now"}, {"desc":"raw(00141226e31987e4bc2e63c0ee12908f675e40464b20)#9pp7qm39","timestamp":"now"}, {"desc":"raw(0014f73f149f7503960a5e849c6ee7a8a8c336f631cb)#qtkxv9fc","timestamp":"now"}, {"desc":"raw(0014c8ccb4d81ffc769fc5fdd8d7eed69b0e0cae5749)#hn39qayv","timestamp":"now"}, {"desc":"raw(001498565aead2d67a22a6021d55210f2a917fc22169)#6ar3vwsx","timestamp":"now"}, {"desc":"raw(001403013248ac0cd9eabe176cad162cda2a19f771e1)#4m47mukd","timestamp":"now"} ]' ``` Running master, when building from souce and using brew installed sqlite, this takes ~3.4s. With this PR, the same operation takes ~0.3s. Github-Pull: #25985 Rebased-From: d216d71 * doc: remove brew install sqlite from macOS docs * Adjust `.tx/config` for new Transifex CLI The old Transifex Command-Line Tool is considered deprecated (as of January 2022) and will sunset on Nov 30, 2022. See: https://github.com/transifex/cli/blob/devel/README.md#migrating-from-older-versions-of-the-client An accompanying PR: bitcoin-core/bitcoin-maintainer-tools#142 Github-Pull: #26321 Rebased-From: d6adbb7 * rpc: fix crash in deriveaddresses when derivation index is 2147483647 2147483647 is the maximum positive value of a signed int32, and - currently - the maximum value that the deriveaddresses bitcoin RPC call accepts as derivation index due to its input validation routines. Before this change, when the derivation index (and thus range_end) reached std::numeric_limits<int_32_t>::max(), the "i" variable in the for cycle (which is declared as int, and as such 32 bits in size on most platforms) would be incremented at the end of the first iteration and then warp back to -2147483648. This caused SIGABRT in bitcoind and a core dump. This change assigns "i" an explicit size of 64 bits on every platform, sidestepping the problem. Fixes #26274. Github-Pull: #26275 Rebased-From: addf9d6 * rpc: add non-regression test about deriveaddresses crash when index is 2147483647 This test would cause a crash in bitcoind (see #26274) if the fix given in the previous commit was not applied. Github-Pull: #26275 Rebased-From: 9153ff3 * build: Bump version to 22.1.0rc1 * doc: Update manual pages for 22.1.0rc1 * doc: update version number in bips.md to v22.1 * qt: 22.1rc1 translations update * vtc review: missed branding items Co-authored-by: Martin Zumsande <[email protected]> Co-authored-by: Pieter Wuille <[email protected]> Co-authored-by: laanwj <[email protected]> Co-authored-by: MacroFake <[email protected]> Co-authored-by: Andrew Chow <[email protected]> Co-authored-by: Hennadii Stepanov <[email protected]> Co-authored-by: fanquake <[email protected]> Co-authored-by: muxator <[email protected]>
Vertcoin-Core 19.0-rc1 is a rebase on upstream Bitcoin-Core 22.x