5ca9b24da1 test: Add makefile target for running unit tests (TheCharlatan)
Pull request description:
`make check` runs a bunch of other subtree tests that exercise code that is hardly ever changed and have a comparatively long runtime. There seems to be no target for running just the unit tests, so add one.
Alternatively the secp256k1 tests could be removed from the `check-local` target, reducing its runtime. This was rejected before though in https://github.com/bitcoin/bitcoin/pull/20264.
ACKs for top commit:
delta1:
utACK 5ca9b24da1
edilmedeiros:
Tested ACK 5ca9b24da1
achow101:
ACK 5ca9b24da1
ryanofsky:
Tested ACK 5ca9b24da1.
Tree-SHA512: 470969d44585d7cc33ad038a16e791db9e2be8469f52ddf122c46f20776fad34e6a48f988861a132c42540158fed05f3cf66fcc3bea05708253daaa35af54339
Ensure we are checking sigop-adjusted virtual size by creating setups
and packages where sigop cost is larger than bip141 vsize.
Co-authored-by: Gregory Sanders <gsanders87@gmail.com>
e064487ca2 addrman, refactor: improve stochastic test in `AddSingle` (brunoerg)
Pull request description:
This PR changes this algorithm to be O(1) instead of O(n). Also, in the current implementation, if `pinfo->nRefCount` is 0, we created an unnecessary variable (`nFactor`), this changes it. the change is relatively simple and does not cause conflicts.
ACKs for top commit:
achow101:
ACK e064487ca2
amitiuttarwar:
ACK e064487ca2
stratospher:
ACK e064487ca2. simple use of << instead of a loop, didn't observe any behaviour difference before and after.
Tree-SHA512: ff0a65155e47f65d2ce3cb5a3fd7a86efef1861181143df13a9d8e59cb16aee9be2f8801457bba8478b17fac47b015bff5cc656f6fac2ccc071ee7178a38d291
fab41697a5 Allow int8_t optimized vector serialization (MarcoFalke)
facaa14785 Faster std::byte (pre)vector (un)serialize (MarcoFalke)
Pull request description:
Currently, large vectors of `std::byte` are (un)serialized byte-by-byte, which is slow. Fix this, by enabling the already existing optimization for them.
On my system this gives a 10x speedup for `./src/bench/bench_bitcoin --filter=PrevectorDeserializeTrivial`, when `std::byte` are used:
```diff
diff --git a/src/bench/prevector.cpp b/src/bench/prevector.cpp
index 2524e215e4..76b16bc34e 100644
--- a/src/bench/prevector.cpp
+++ b/src/bench/prevector.cpp
@@ -17,7 +17,7 @@ struct nontrivial_t {
static_assert(!std::is_trivially_default_constructible<nontrivial_t>::value,
"expected nontrivial_t to not be trivially constructible");
-typedef unsigned char trivial_t;
+typedef std::byte trivial_t;
static_assert(std::is_trivially_default_constructible<trivial_t>::value,
"expected trivial_t to be trivially constructible");
```
However, the optimization does not cover `signed char`. Fix that as well.
ACKs for top commit:
sipa:
utACK fab41697a5
achow101:
ACK fab41697a5
TheCharlatan:
ACK fab41697a5
Tree-SHA512: a3e20f375fd1d0e0dedb827a8ce528de1173ea69660c8c891ad1343a86b422072f6505096fca0d3f8af4442fbe1378a02e32d5974919d4e88ff06934d0258cba
86960cdb7f wallet: migration, batch addressbook records removal (furszy)
342c45f80e wallet: addressbook migration, batch db writes (furszy)
595bbe6e81 refactor: wallet, simplify addressbook migration (furszy)
d0943315b1 refactor: SetAddressBookWithDB, minimize number of map lookups (furszy)
bba4f8dcb5 refactor: SetAddrBookWithDB, signal only if write succeeded (furszy)
97b0753923 wallet: clean redundancies in DelAddressBook (furszy)
Pull request description:
Commits decoupled from #28574, focused on the address book cloning process
Includes:
1) DB batch operations and flow simplification for the address book migration process.
2) Code improvements to `CWallet::DelAddressBook` and `Wallet::SetAddrBookWithDB` methods.
These changes will let us consolidate all individual write operations that take place during the wallet migration process into a single db txn in the future.
ACKs for top commit:
achow101:
ACK 86960cdb7f
josibake:
reACK 86960cdb7f
Tree-SHA512: 10c941df3cd84fd8662b9c9ca6a1ed2c7402d38c677d2fc66b8b6c9edc6d73e827a5821487bbcacb5569d502934fa548fd10699e2ec45185f869e43174d8b2a1
cfcb9b1ecf test: wallet, coverage for concurrent db transactions (furszy)
548ecd1155 tests: Test for concurrent writes with db tx (Ava Chow)
395bcd2454 sqlite: Ensure that only one SQLiteBatch is writing to db at a time (Ava Chow)
Pull request description:
The way that we have configured SQLite to run means that only one database transaction can be open at a time. Typically, each individual read and write operation will be its own transaction that is opened and committed automatically by SQLite. However, sometimes we want these operations to be batched into a multi-statement transaction, so `SQLiteBatch::TxnBegin`, `SQLiteBatch::TxnCommit`, and `SQLiteBatch::TxnAbort` are used to manage the transaction of the database.
However, once a db transaction is begun with one `SQLiteBatch`, any operations performed by another `SQLiteBatch` will also occur within the same transaction. Furthermore, those other `SQLiteBatch`s will not be expecting a transaction to be active, and will abort it once the `SQLiteBatch` is destructed. This is problematic as it will prevent some data from being written, and also cause the `SQLiteBatch` that opened the transaction in the first place to be in an unexpected state and throw an error.
To avoid this situation, we need to prevent the multiple batches from writing at the same time. To do so, I've implemented added a `CSemaphore` within `SQLiteDatabase` which will be used by any `SQLiteBatch` trying to do a write operation. `wait()` is called by `TxnBegin`, and at the beginning of `WriteKey`, `EraseKey`, and `ErasePrefix`. `post()` is called in `TxnCommit`, `TxnAbort` and at the end of `WriteKey`, `EraseKey`, and `ErasePrefix`. To avoid deadlocking on ` TxnBegin()` followed by a `WriteKey()`, `SQLiteBatch will now also track whether a transaction is in progress so that it knows whether to use the semaphore.
This issue is not a problem for BDB wallets since BDB uses WAL and provides transaction objects that must be used if an operation is to occur within a transaction. Specifically, we either pass a transaction pointer, or a nullptr, to all BDB operations, and this allows for concurrent transactions so it doesn't have this problem.
Fixes #29110
ACKs for top commit:
josibake:
ACK cfcb9b1ecf
furszy:
ACK cfcb9b1ecf
ryanofsky:
Code review ACK cfcb9b1ecf. This looks great and I think it is ready for merge. Just holding off because josibake seemed ready to review https://github.com/bitcoin/bitcoin/pull/29112#issuecomment-1930372190 and might have more feedback.
Tree-SHA512: 2dd5a8e76df52451a40e0b8a87c7139d68a0d8e1bf2ebc79168cc313e192dab87cfa4270ff17fea4f7b370060d3bc9b5d294d50f7e07994d9b5a69b40397c927
Instead of doing one db transaction per removed record,
we now batch all removals in a single db transaction.
Speeding up the process and preventing the wallet from entering
an inconsistent state when any of the intermediate writes fail.
1) Encode destination only once (instead of three).
2) Fail if the entry's linked data cannot be removed.
3) Don't remove entry from memory if db write fail.
4) Notify GUI only if removal succeeded
a17fd33edd GUI: OptionsDialog: Replace verbose two-option font selector with simple combobox with Custom... choice (Luke Dashjr)
98e9ac5199 GUI: Use FontChoice type in OptionsModel settings abstraction (Luke Dashjr)
3a6757eed9 GUI: Load custom FontForMoney from QSettings (Luke Dashjr)
49eb97eff9 GUI: Add possibility for an explicit QFont for FontForMoney in OptionsModel (Luke Dashjr)
f2dfde80b8 GUI: Move "embedded font or not" decision into new OptionsModel::getFontForMoney method (Luke Dashjr)
Pull request description:
This replaces the overly-verbose radio-button font setting (which only allows embedded or autodetected from system) with a simple combobox providing both existing options as well as a custom option to allow the user to select any font of their choice/style.
ACKs for top commit:
pablomartin4btc:
tested ACK a17fd33edd
hebasto:
ACK a17fd33edd, I have reviewed the code and tested it on Ubuntu 22.04. This is a UX improvement. https://github.com/bitcoin-core/gui/pull/497#issuecomment-1341222673 might be addressed in a follow-up.
Tree-SHA512: 2f0a8bc1242a374c4b7dc6e34014400428b6d36063fa0b01c9f62a8bd6078adfbbca93d95c87e4ccb580d982fe10173e1d9a28bcec586591dd3f966c7b90fc5d
b14298c5bc fuzz: remove unused `args` and `context` from `FuzzedWallet` (brunoerg)
Pull request description:
`ArgsManager args` and `WalletContext context` were previously used to create the wallet into `FuzzedWallet`. After fa15861763, they are not used anymore. This PR removes them.
ACKs for top commit:
maflcko:
lgtm ACK b14298c5bc
epiccurious:
utACK b14298c5bc
ryanofsky:
Code review ACK b14298c5bc
Tree-SHA512: 164e6a66ba05e11176a0cf68db6257f0ac07459cf7aa01ec4302b303c156c205a68128373a0b8daba0a6dfbff990af7fa14465a6341a296312fb20ea778c7a8c
e2ad343f69 wallet: remove unused `SignatureData` instances in spkm's `FillPSBT` methods (Sebastian Falbesoner)
Pull request description:
These are filled with signature data from a PSBT input, but not used anywhere after, hence they can be removed. Note that the same code is in the `SignPSBTInput` function where the `sigdata` result is indeed used.
ACKs for top commit:
achow101:
ACK e2ad343f69
brunoerg:
crACK e2ad343f69
Tree-SHA512: f0cabcc000bcea6bc7d7ec9d3be2e2a8accbdbffbe35252250ea2305b65a5813fde2b8096fbdd2c7cccdf417ea285183dc325fc2d210d88bce62978ce642930e
2bb25ce502 wallet: remove unused 'accept_no_keys' arg from decryption process (furszy)
Pull request description:
Found it while reviewing other PR. Couldn't contain myself from cleaning it up.
The wallet decryption process (`CheckDecryptionKey()` and `Unlock()`)
contains an arg 'accept_no_keys,' introduced in #13926, that has
never been used.
Additionally, this also removes the unimplemented `SplitWalletPath`
function.
ACKs for top commit:
delta1:
ACK 2bb25ce502
epiccurious:
utACK 2bb25ce502.
achow101:
ACK 2bb25ce502
theStack:
Code-review ACK 2bb25ce502
Tree-SHA512: e0537c994be19ca0032551d8a64cf1755c8997e04d21ee0522b31de26ad90b9eb02a8b415448257b60bced484b9d2a23b37586e12dc5ff6e35bdd8ff2165c6bf
A SQLiteBatch need to wait for any other batch to finish writing before
it can begin writing, otherwise db txn state may be incorrectly
modified. To enforce this, each SQLiteDatabase has a semaphore which
acts as a lock and is acquired by a batch when it begins a write, erase,
or a transaction, and is released by it when it is done.
To avoid deadlocking on itself for writing during a transaction,
SQLiteBatch also keeps track of whether it has begun a transaction.
e7fd70f4b6 [test] make v2transport arg in addconnection mandatory and few cleanups (stratospher)
Pull request description:
- make `v2transport` argument in `addconnection` regression-testing only RPC mandatory. https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1470738750
- previously it was an optional arg with default `false` value.
- only place this RPC is used is in the [functional tests](11b436a66a/test/functional/test_framework/test_node.py (L742)) where we always pass the appropriate `v2transport` option to the RPC anyways. (and that too just for python dummy peer(`P2PInterface`) and bitcoind(`TestNode`) interactions)
- rename `v2_handshake()` to `_on_data_v2_handshake()` https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1466958424
- more compact return statement in `wait_for_reconnect()` https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1466979708
- assertion to check that empty version packets are received from `TestNode`.
ACKs for top commit:
glozow:
ACK e7fd70f4b6
theStack:
Code-review ACK e7fd70f4b6
mzumsande:
Code Review ACK e7fd70f4b6
Tree-SHA512: e66e29baccd91e1e4398b91f7d45c5fc7c2841d77d8a6178734586017bf2be63496721649da91848dec71da605ee31664352407d5bb896e624cc693767c61a1f
c819a83b4d Don't use scientific notation in log messages (Kristaps Kaupe)
Pull request description:
Don't see any benefits here, only harder to read for most of the users.
Before:
```
2024-01-16T13:11:36Z Dumped mempool: 8.165e-06s to copy, 0.00224268s to dump
```
After:
```
2024-01-16T13:11:36Z Dumped mempool: 0.000s to copy, 0.002s to dump
```
ACKs for top commit:
kristapsk:
> > > > lgtm ACK [c819a83](c819a83b4d). can you update the PR description?
glozow:
lgtm ACK c819a83b4d. can you update the PR description?
Tree-SHA512: 0972e0a05934e1b014fdeca0c235065aa017ba9abf74b3018f514e4d8022ef02b7f042a07d3675144b51449492468aea6b5b0183233ad7f1bab887d18e3d06af
fa5cd66f0a test: Assumeutxo with more than just coinbase transactions (MarcoFalke)
Pull request description:
Currently the AU tests only check that loading a txout set with only coinbase outputs works.
Fix that by adding other transactions.
ACKs for top commit:
jamesob:
ACK fa5cd66f0a
glozow:
concept ACK fa5cd66f0a
Tree-SHA512: e090c41f73490ad72e36c478405bfd0716d46fbf5a131415095999da6503094a86689a179a84addae3562b760df64cdb67488f81692178c8ca8bf771b1e931ff
make check runs a bunch of other subtree tests that exercise code that
is hardly ever changed and have a comparatively long runtime. There
seems to be no target for running just the unit tests, so add one.
The wallet decryption process (CheckDecryptionKey() and Unlock())
contains an arg 'accept_no_keys,' introduced in #13926, that has
never been used.
Additionally, this also removes the unimplemented SplitWalletPath
function.
4da76ca247 test: Test migration of tx with both spendable and watchonly (Ava Chow)
c62a8d03a8 wallet: Keep txs that belong to both watchonly and migrated wallets (Ava Chow)
71cb28ea8c test: Make sure that migration test does not rescan on reloading (Ava Chow)
78ba0e6748 wallet: Reload the wallet if migration exited early (Ava Chow)
9332c7edda wallet: Write bestblock to watchonly and solvable wallets (Ava Chow)
Pull request description:
A transaction does not necessarily have to belong to either the migrated wallet (with the private keys) and the watchonly wallet (with watchonly things), it could have multiple outputs with each isminetype. So we should be putting such transactions in one or the other wallet, but rather putting it in both.
I've added a test for this behavior, however the test also revealed a few other issues. Notably, it revealed that `migratewallet` would have the watchonly wallet rescan from genesis when it is reloaded at the end of migration. This could be a cause for migration appearing to be very slow. This is resolved by first writing best block records to the watchonly and solvable wallets, as well as updating the test to make sure that rescans don't happen.
The change to avoid rescans also found an issue where some of our early exits would result in unloading the wallet even though nothing happened. So there is also a commit to reload the wallet for such early exits.
ACKs for top commit:
ryanofsky:
Code review ACK 4da76ca247. This looks great. The code is actually cleaner than before, two bugs are fixed, and the test checking for rescanning is pretty clever and broadens test coverage.
furszy:
Code review ACK 4da76ca2
Tree-SHA512: 5fc210cff16ca6720d7b2d0616d7e3f295c974147854abc704cf99a3bfaad17572ada084859e7a1b1ca94da647ad130303219678f429b7995f85e040236db35c
3904123da9 tests: Test that descriptors flag is set for migrated blank wallets (Ava Chow)
072d506240 wallet: Make sure that the descriptors flag is set for blank wallets (Ava Chow)
Pull request description:
While rebasing #28710 after #28976 was merged, I realized that although blank wallets were being moved to sqlite, `WALLET_FLAG_DESCRIPTORS` was not being set so those blank wallets would still continue to be treated as legacy wallets.
To fix that, just set the descriptor flags for blank wallets. Also added a test to catch this.
ACKs for top commit:
epiccurious:
Tested ACK 3904123da9.
delta1:
tested ACK 3904123da9
ryanofsky:
Code review ACK 3904123da9
murchandamus:
code review ACK 3904123da9
Tree-SHA512: 9f6fe9c1899ca387ab909b1bb6956cd6bc5acbf941686ddc6c061f9b1ceec2cc9d009ff472486fc86e963f6068f0e2f1ae96282e7c630193797a9734c4f424ab
Requesting historical blocks from network limited peers is a
direct disconnection cause.
The node must only request the blocks who know for sure the
limited peer can provide.
It is possible for a transaction that has an output that belongs to the
mgirated wallet, and another output that belongs to the watchonly
wallet. Such transaction should appear in both wallets during migration.
Migration will unload loaded wallets prior to beginning. It will then
perform some checks which may exit early. Such unloaded wallets should
be reloaded prior to exiting.
When migrating, we should also be writing the bestblock record to the
watchonly and solvable wallets to avoid rescanning on the reload as that
can be slow.
fad74bbbd0 refactor: Mark prevector iterator with std::contiguous_iterator_tag (MarcoFalke)
fab8a01048 refactor: Fix binary operator+ for prevector iterators (MarcoFalke)
fa44a60b2b refactor: Fix constness for prevector iterators (MarcoFalke)
facaa66b49 refactor: Add missing default constructor to prevector iterators (MarcoFalke)
Pull request description:
Currently prevector iterators have many issues:
* Forward iterators (and stronger) must be default constructible (https://eel.is/c++draft/forward.iterators#1.2). Otherwise, some functions can not be instantiated, like `std::minmax_element`.
* Various `const` issues with random access iterators. For example, a `const iterator` is different from a `const_iterator`, because the first one holds a mutable reference and must also return it without `const`. Also, `operator+` must be callable regardless of the iterator object's `const`-ness.
* When adding an offset to random access iterators, both `x+n` and `n+x` must be specified, see https://eel.is/c++draft/random.access.iterators#tab:randomaccessiterator
Fix all issues.
Also, upgrade the `std::random_access_iterator_tag` (C++17) to `std::contiguous_iterator_tag` (C++20)
ACKs for top commit:
TheCharlatan:
ACK fad74bbbd0
stickies-v:
ACK fad74bbbd0
willcl-ark:
ACK fad74bbbd0
Tree-SHA512: b1ca778a31602af94b323b8feaf993833ec78be09f1d438a68335485a4ba97f52125fdd977ffb9541b89f8d45be0105076aa07b5726936133519aae832556e0b
b851c5385d fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses (Vasil Dimov)
Pull request description:
In the process of doing so, refactor `ConsumeNetAddr()` to generate the addresses from IPv4, IPv6, Tor, I2P and CJDNS networks in the same way - by preparing some random stream and deserializing from it. Similar code was already found in `RandAddr()`.
ACKs for top commit:
achow101:
ACK b851c5385d
mzumsande:
ACK b851c5385d
brunoerg:
utACK b851c5385d
Tree-SHA512: 9905acff0e996f30ddac0c14e5ee9e1db926c7751472c06d6441111304242b563f7c942b162b209d80e8fb65a97249792eef9ae0a96100419565bf7f59f59676
987a1b51ee init: settings, do not load auto-generated warning msg (furszy)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/pull/29144#issuecomment-1907071391.
The settings warning message is meant to be used only to discourage users from
modifying the file manually. Therefore, there is no need to keep it in memory.
ACKs for top commit:
achow101:
ACK 987a1b51ee
ryanofsky:
Code review ACK 987a1b51ee. Seems like a clean, simple fix
Tree-SHA512: 3f2bdcf4b4a9cadb396dcff9b43155211eeed018527a07356970a341d139ad18edbd1a4d369377c8907b8ec1f19ee2ab8aacf85a887379e6d57a8a6db2403d51
c11c404281 tests: Test migration of blank wallets (Andrew Chow)
563b2a60d6 wallet: Better error message when missing LegacySPKM during migration (Andrew Chow)
b1d2c771d4 wallet: Check for descriptors flag before migration (Andrew Chow)
8c127ff1ed wallet: Skip key and script migration for blank wallets (Andrew Chow)
Pull request description:
Blank wallets (wallets without any keys are scripts) are being detected as already being descriptor wallets even though they are not. This is because the check for whether a wallet is already a descriptor wallet uses the presence of a `LegacyScriptPubKeyMan` which is only setup when keys or scripts are found. This PR resolves this issue by checking for the descriptor wallet flag instead and subsequently skipping the keys and scripts part of migration for blank wallets.
Fixes the issue mentioned in https://github.com/bitcoin/bitcoin/pull/28868#issuecomment-1809641110
ACKs for top commit:
furszy:
reACK c11c404281. CI failure is unrelated.
ryanofsky:
Code review ACK c11c404281
Tree-SHA512: 2466fdf1542eb8489c841253191f85dc88365493f0bb3395b67dee3e43709a9993c68b9d7623657b54b779adbe68fc81962d60efef4802c5d461f154167af7f4
ff9039f6ea Remove GetAdjustedTime (dergoegge)
Pull request description:
This picks up parts of #25908.
The use of adjusted time is removed from validation code while the warning to users if their clock is out of sync with the rest of the network remains.
ACKs for top commit:
naumenkogs:
ACK ff9039f6ea
achow101:
ACK ff9039f6ea
maflcko:
lgtm ACK ff9039f6ea🤽
stickies-v:
ACK ff9039f6ea
Tree-SHA512: d1f6b9445c236915503fd2ea828f0d3b92285a5dbc677b168453276115e349972edbad37194d8becd9136d8e7219b576af64ec51c72bdb1923e57e405c0483fc
0bef1042ce net: enable v2transport by default (Pieter Wuille)
Pull request description:
This enables BIP324's v2 transport by default (see #27634):
* Inbound connections will auto-sense whether v1 or v2 is in use.
* Automatic outbound connections will use v2 if `NODE_P2P_V2` was set in addr gossip, but retry with v1 if met with immediate failure.
* Manual outbound connections will default to v2, but retry with v1 if met with immediate failure.
It remains possible to run with `-v2transport=0` to disable all of these, and make all outbound and inbound connections v1. It also remains possible to specify the `v2transport` argument to the `addnode` RPC as `false`, to disable attempting a v2 connection for that particular added node.
ACKs for top commit:
stratospher:
ACK 0bef104.
josibake:
reACK 0bef1042ce
achow101:
ACK 0bef1042ce
naumenkogs:
ACK 0bef1042ce
theStack:
ACK 0bef1042ce
willcl-ark:
crACK 0bef1042ce
BrandonOdiwuor:
utACK 0bef1042ce
pablomartin4btc:
re ACK 0bef1042ce
kristapsk:
utACK 0bef1042ce
Tree-SHA512: 3f17a91e318b9304c40c74a7a5b231149f664ae684d13e9739a05be6c05ba9720f3c3c62da6a73ace0ae8ce733f1c8410b211f9fa15694e6a8d28999ab9882d8
b298242c8d test: sqlite, add coverage for dangling to-be-reverted db txns (furszy)
fc0e747192 sqlite: guard against dangling to-be-reverted db transactions (furszy)
472d2ca981 sqlite: introduce HasActiveTxn method (furszy)
dca874e838 sqlite: add ability to interrupt statements (furszy)
fdf9f66909 test: wallet db, exercise deadlock after write failure (furszy)
Pull request description:
Discovered while was reviewing #29112, specifically https://github.com/bitcoin/bitcoin/pull/29112#pullrequestreview-1821862931.
If the db handler that initiated the database transaction is destroyed,
the ongoing transaction cannot be left dangling when the db txn fails
to abort. It must be forcefully reverted; otherwise, any subsequent
db handler executing a write operation will dump the dangling,
to-be-reverted transaction data to disk.
This not only breaks the isolation property but also results in the
improper storage of incomplete information on disk, impacting
the wallet consistency.
This PR fixes the issue by resetting the db connection, automatically
rolling back the transaction (per https://www.sqlite.org/c3ref/close.html)
when the handler object is being destroyed and the txn abortion failed.
Testing Notes
Can verify the failure by reverting the fix e5217fea and running the test.
It will fail without e5217fea and pass with it.
ACKs for top commit:
achow101:
ACK b298242c8d
ryanofsky:
Code review ACK b298242c8d. Just fix for exec result codes and comment update since last review.
Tree-SHA512: 44ba0323ab21440e79e9d7791bc1c56a8873c8bd3e8f6a85641b91576e1293011fa8032d8ae5b0580f3fb7a949356f7b9676693d7ceffa617aaad9f6569993eb
