fa9f4554ca5e64e7960b74dd0ad8fb1cd7c2f091 refactor: Remove pointless and confusing shift in RelayAddress (MarcoFalke)
Pull request description:
The second argument written to the siphash is already quantized to 24 hours, so it seems confusing to quantize the first argument to 32 bits (out of 64 bits).
> The shifting is pointless, we should get rid of it. It seems to be a silly evolution of this 2010 Satoshi code: 5cbf753 (where it made sense because everything was XORed together, and the address used the high bits, while the time used the low ones).
(Copied from https://github.com/bitcoin/bitcoin/pull/18642#issuecomment-613773120)
(The original code was `uint256 hashRand = hashSalt ^ (((int64)addr.ip)<<32) ^ ((GetTime()+addr.ip)/(24*60*60));`)
This also allows to remove a integer sanitizer suppression for the whole file.
ACKs for top commit:
laanwj:
Code review ACK fa9f4554ca5e64e7960b74dd0ad8fb1cd7c2f091
sipa:
utACK fa9f4554ca5e64e7960b74dd0ad8fb1cd7c2f091
promag:
Code review ACK fa9f4554ca5e64e7960b74dd0ad8fb1cd7c2f091.
Tree-SHA512: f5fd107464ccd839d6749aed6914b4935e39ab42906546b3f3810a7339fc4633fef931a1783a287572af5ec64525626fa91d147d8ff52eb076740465bf5cf839
ac617cc141fe05bea0dc5e8f9df3da43c0945842 wallettool: Check that the dumpfile checksum is the correct size (Andrew Chow)
Pull request description:
After parsing the checksum, make sure that it is the size that we expect it to be.
This issue was reported by Pedro Baptista.
ACKs for top commit:
laanwj:
Code review ACK ac617cc141fe05bea0dc5e8f9df3da43c0945842
Tree-SHA512: 8135b3fb1f4f6b6c91cfbac7d1d3421f1f6c664a742c92940f68eae857f92ce49d042cc3aa5c2df6ef182825271483d65efc7543ec7a8ff047fd7c08666c8899
8bd34dc774788cbf3cad8e139542a0ed9f3e8bb4 test: check that bitcoin-tx detects missing input amount for segwit transactions (Sebastian Falbesoner)
c337b27d7cfd468048bcf816e585a1f7d59e066d Require that input amount is provided for bitcoin-tx witness transactions (Ben Woosley)
Pull request description:
This PR picks up the obviously abandoned PR #13608 (last activity was three and a half years ago) by rebasing it on master and adding missing tests. Original PR description: "_Applies fix from #12458 / #13547 to bitcoin-tx._"
The private key is the compressed version of the one used in most other util tests (5HpHagT65TZzG1PH3CSu63k8DbpvD8s5ip4nEB3kEsreAnchuDf, corresponds to the scalar value k=1 in big endian), since segwit signing refuses uncompressed keys.
The error message from the picked up PR is changed to not include the amount, as showing any value would be just confusing.
ACKs for top commit:
josibake:
ACK 8bd34dc774
Tree-SHA512: 334b418f89527363ad7e3326b4126e86a05fd64876c49a8280de38e64cfac52cb62c4b24b83603dd68b6bcebbe57c64161832edffb1cac7e9c68426f6b6eae1f
fab16415ba1729748b03518c92396febda5a25d1 doc: Fix typo in getmempoolinfo (MarcoFalke)
Pull request description:
Also, remove whitespace. Can be reviewed with `--ignore-all-space --word-diff-regex=.`.
ACKs for top commit:
laanwj:
Good catch. ACK fab16415ba1729748b03518c92396febda5a25d1
shaavan:
ACK fab16415ba1729748b03518c92396febda5a25d1
Tree-SHA512: 9d51ef4a4eccfcf437c99a9f84f48e4f090d75715332ad2b4cf10ad77c3691de03255b4817e9fd203fad9baf338066982304dc62fab93dd605e735943f8ca346
6bf6e9fd9dece67878595a5f3361851c25833c49 net: change CreateNodeFromAcceptedSocket() to take Sock (Vasil Dimov)
9e3cbfca7c9efa620c0cce73503772805cc1fa82 net: use Sock in CConnman::ListenSocket (Vasil Dimov)
f8bd13f85ae5404adef23a52719d804a5c36b1e8 net: add new method Sock::Accept() that wraps accept() (Vasil Dimov)
Pull request description:
_This is a piece of https://github.com/bitcoin/bitcoin/pull/21878, chopped off to ease review._
Introduce an `accept(2)` wrapper `Sock::Accept()` and extend the usage of `Sock` in `CConnman::ListenSocket` and `CreateNodeFromAcceptedSocket()`.
ACKs for top commit:
laanwj:
Code review ACK 6bf6e9fd9dece67878595a5f3361851c25833c49
jamesob:
ACK 6bf6e9fd9dece67878595a5f3361851c25833c49 ([`jamesob/ackr/21879.2.vasild.wrap_accept_and_extend_u`](https://github.com/jamesob/bitcoin/tree/ackr/21879.2.vasild.wrap_accept_and_extend_u))
jonatack:
ACK 6bf6e9fd9dece67878595a5f3361851c25833c49 per `git range-diff ea989de 976f6e8 6bf6e9f` -- only change since my last review was `s/listen_socket.socket/listen_socket.sock->Get()/` in `src/net.cpp: CConnman::SocketHandlerListening()` -- re-read the code changes, rebase/debug build/ran units following my previous full review (https://github.com/bitcoin/bitcoin/pull/21879#pullrequestreview-761251278)
w0xlt:
tACK 6bf6e9f
Tree-SHA512: dc6d1acc4f255f1f7e8cf6dd74e97975cf3d5959e9fc2e689f74812ac3526d5ee8b6a32eca605925d10a4f7b6ff1ce5e900344311e587d19786b48c54d021b64
fa996c58e8a31ebe610d186cef408b6dd3b385a8 refactor: Avoid integer overflow in ApplyStats when activating snapshot (MarcoFalke)
fac01888d17423d6c23a9ce15d98fc88fb34e3cc Move AdditionOverflow to util, Add CheckedAdd with unit tests (MarcoFalke)
fa526d8fb6ab8f2678a30d4536aa9c45218f5269 Add dev doc to CCoinsStats::m_hash_type and make it const (MarcoFalke)
faff051560552d4405896e01920a18f698155a56 style: Remove unused whitespace (MarcoFalke)
Pull request description:
A snapshot contains the utxo set, including the out value. To activate the snapshot, the hash needs to be calculated. As a side-effect, the total amount in the snapshot is calculated (as the sum of all out values), but never used. Instead of running into an integer overflow in an unused result, don't calculate the result in the first place.
Other code paths (using the active utxo set) can not run into an integer overflow, since the active utxo set is valid.
Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=39716
ACKs for top commit:
shaavan:
reACK fa996c58e8a31ebe610d186cef408b6dd3b385a8
vasild:
ACK fa996c58e8a31ebe610d186cef408b6dd3b385a8
Tree-SHA512: 4f207f634841f6f634fd02ae1e5907e343fd767524fd0e8149aa99fa9a1834fe50167d14874834d45236e9c325d567925f28129bacb7d80be29cf22277a16a14
faa51a6aa92966403b1c46d4047f5b3aafcccd1d doc: Mark proprietary array optional (MarcoFalke)
Pull request description:
The global one is returned all the time, but the input/output array is returned optionally
ACKs for top commit:
fanquake:
ACK faa51a6aa92966403b1c46d4047f5b3aafcccd1d
Tree-SHA512: db987c13d59e0ccc633032707438d506fe4f8fbf7569a03b99d899cb1309de94f99c343840107fc51a9f904bcf55e00049808b6cdf732fc16c6e9e818b480936
ea4c9fd4ab9aaa2e8f2c2e38a75c9f05d0bfc866 test: Cover eviction by timeout in addrman_evictionworks (Martin Zumsande)
4f1bb467b556ec93c9b8f758783fda4d050da491 test: Add test for multiplicity in addrman new tables (Martin Zumsande)
e880bb7836dab2018049390884220177c6db9b92 test: Add test for updating addrman entries (Martin Zumsande)
f02eee8c8784dfc8db80a21ab6508f7c99298255 test: introduce utility function to retrieve an addrman (Martin Zumsande)
f0e5efb82493f7a14580335ce719d5be81c8713e test: Remove unused AddrManTest class (Martin Zumsande)
b696d7870b29232057600df5ddd8351888253b95 test: Remove tests for internal helper functions (Martin Zumsande)
0538520091bf2982a029a0298835400f5afbdc15 test: use AddrMan instead of AddrManTest where possible (Martin Zumsande)
1c65d427bbf61bb558cf7e18f7aff99b19f68508 test: Inline SimConnFail function (Martin Zumsande)
5b7aac34f2363822c3a1cfafda8ffc9528905058 test: delete unused GetBucketAndEntry function (Amiti Uttarwar)
2ba1e74e59a325ca6cb140757067dd5e0c7c249b test: Update addrman_serialization unit test to use AddrMan's interface (Amiti Uttarwar)
dad5f760211df314d650999e0a76edb0151b4fe1 addrman: Introduce a test-only function to lookup addresses (Amiti Uttarwar)
Pull request description:
This PR (joint work with Amiti Uttarwar) changes the addrman unit tests such that they only use the public `AddrMan` interface:
This has the advantage that the tests are less implementation-dependent, i.e. it would be possible to rewrite the internal addrman implementation (as drafted [here](https://github.com/sipa/bitcoin/tree/202106_multiindex_addrman) for using a multiindex) without having to adjust the tests.
This includes the following steps:
* Adding a test-only function `FindAddressEntry()` to the public addrman interface which returns info about an address in addrman (e.g. bucket, position, whethe the address is in new or tried). Obviously we want to do this sparingly, but I think a single test-only function is ok (which could also be useful elsewhere, e.g. in fuzz tests).
* Removal of the `AddrManTest` subclass which would reach into AddrMan's internals, using `AddrMan` instead
* Removal of tests for internal helper functions that are not publicly exposed (these are still tested indirectly via the public functions calling them).
* Additional tests for previously untested features such as multiplicity in the new tables, that can be tested with the help of `FindAddressEntry()`.
All in all, this PR increases the unit test coverage of AddrMan by a bit.
ACKs for top commit:
jnewbery:
ACK ea4c9fd4ab9aaa2e8f2c2e38a75c9f05d0bfc866
josibake:
reACK ea4c9fd4ab
Tree-SHA512: c2d4ec8bdc62ffd6055ddcd37dea85ec08c76889e9e417e8d7c62a96cf68a8bcbe8c67bec3344d91fa7d3c499f6d9f810962da1dddd38e70966186b10b8ab447
fa5bb37611dea5d3e6715b0c84249e39ad1d2476 rpc: Use EnsureAnyArgsman in rpc/blockchain (MarcoFalke)
fa98b6f6449d33b315a73156c6de968f1757d53a rpc: Add EnsureArgsman helper (MarcoFalke)
Pull request description:
This refactor doesn't change anything for the rpc layer, but it helps a bit with removing gArgs (See #21005)
ACKs for top commit:
shaavan:
Code Review ACK fa5bb37611dea5d3e6715b0c84249e39ad1d2476
Tree-SHA512: 18f9cc90830a168acb94452b541b6e97ba9a50a0f4834698a16994c3884c833dc606e36a5538a16352e5e5cc43d3ac77cb498f877f8f5bc5dd52fa84f8bf2056
059f88b6a97b7a3ae1f033885e40ac01f91e6d60 Add RPC help for getblock verbosity level 3 (Kiminuo)
1bdd5f63229ebf28c24a8656f486ed8a6c8b3787 Address review comments from #22918 (Kiminuo)
Pull request description:
This is a follow-up PR to #22918 which addresses review comments (first commit). The second commit adds missing RPC help for verbosity level 3.
ACKs for top commit:
pg156:
ACK 059f88b6a9
laanwj:
re-ACK 059f88b6a97b7a3ae1f033885e40ac01f91e6d60
Tree-SHA512: f27d53ac34b93a304ef5668701ed2b5c986a926bc8ad0df4de89695fc9e1df26acb008611451319ea897658acd9c56c6a0555d60359960c9cd28238ebefa2d50
1831d43e8fb73358858907e1d95ce54e1f42b2e7 Change 'Show' icon (w0xlt)
Pull request description:
This PR changes the 'Show' icon in `receivecoinsdialog.{ui,cpp}`.
The current icon for the 'Show' button is the edit icon, which makes it look like records can be edited on this screen (which is not the case).
The icon already available that seems to be the most suitable for this case is the "eye", so this PR makes this change.
| PR | Master |
| ------------- | ------------- |
| <img width="209" alt="PR" src="https://user-images.githubusercontent.com/94266259/147833993-0758291c-af87-49a8-ae20-7fb9f944cb38.png"> | <img width="209" alt="master" src="https://user-images.githubusercontent.com/94266259/147833992-30d7549d-be7b-4479-8bca-314810e3adb8.png"> |
ACKs for top commit:
kristapsk:
utACK 1831d43e8fb73358858907e1d95ce54e1f42b2e7
shaavan:
ACK 1831d43e8fb73358858907e1d95ce54e1f42b2e7
hebasto:
ACK 1831d43e8fb73358858907e1d95ce54e1f42b2e7
Tree-SHA512: 45e2c0ed51626b40de486903602f2e84a74ff8f09d84de8412c30103d4e15fcddaddbc40571f653da487c538feca33411cf07ad7dd21a9a774bfd45b171330f4
fa7efc915b87ec56ca1cc0bad7d8f79591bfa099 Fixup style of moved code (MarcoFalke)
fade2a44f4aabc64185031dbf4c70d875ece6740 Move BlockManager to node/blockstorage (MarcoFalke)
Pull request description:
`BlockManager` is responsible for reading and writing block(headers). So move it to the existing `blockstorage` module in `node`. Also, move validation code unrelated to block-storage out from `BlockManager`.
ACKs for top commit:
ryanofsky:
Code review obvious ACK fa7efc915b87ec56ca1cc0bad7d8f79591bfa099
Tree-SHA512: 0197943d818e5f59e743b07fbb92e7661bff90081127a41e35e5692ce49d6f6a7872448670b0da282f7714580a45c8d93e571a67177c8b5f785ce9edefe834c5
29e1794ba5350914ea2be8cba33d8d6d2c99760b build, qt: No need to set inapplicable QPA backend for Android (Hennadii Stepanov)
Pull request description:
The current workflow looks weird. At first, the inapplicable `xcb` QPA backend is set in Qt `configure` options. Then the correct `android` QPA backend is forced via the `QT_QPA_PLATFORM` environment variable.
Using the default QPA backend, which is `android` for Android devices, is just fine.
ACKs for top commit:
icota:
re-tACK 29e1794ba5350914ea2be8cba33d8d6d2c99760b
Tree-SHA512: 08ed7d05209c1bedc1a71de5ea3be5d86b40319a164dceb9191f7a4dfe78f2f951778b90421335e73e71a798a57bdf046aa96876762d338b600037bd7ee27b52
b4adc5ad6769e4a5a6179dfff271cd4c9dc47a5b [bugfix] update lockpoints correctly during reorg (glozow)
b6002b07a36f0d58dc6becd04bfcf78599056b7c MOVEONLY: update_lock_points to txmempool.h (glozow)
Pull request description:
I introduced a bug in #22677 (sorry! 😅)
Mempool entries cache `LockPoints`, containing the first height/blockhash/`CBlockIndex*` at which the transaction becomes valid. During a reorg, we re-check timelocks on all mempool entries using `CheckSequenceLocks(useExistingLockPoints=false)` and remove any now-invalid entries. `CheckSequenceLocks()` also mutates the `LockPoints` passed in, and we update valid entries' `LockPoints` using `update_lock_points`. Thus, `update_lock_points(lp)` needs to be called right after `CheckSequenceLocks(lp)`, otherwise we lose the data in `lp`. I incorrectly assumed they could be called in separate loops.
The incorrect behavior introduced is: if we have a reorg in which a timelocked mempool transaction is still valid but becomes valid at a different block, the cached `LockPoints` will be incorrect.
This PR fixes the bug, adds a test, and adds an assertion at the end of `removeForReorg()` to check that all mempool entries' lockpoints are valid. You can reproduce the bug by running the test added in the [test] commit on the code before the [bugfix] commit.
ACKs for top commit:
jnewbery:
ACK b4adc5ad6769e4a5a6179dfff271cd4c9dc47a5b
vasild:
ACK b4adc5ad6769e4a5a6179dfff271cd4c9dc47a5b
mzumsande:
Code Review ACK b4adc5ad6769e4a5a6179dfff271cd4c9dc47a5b
hebasto:
ACK b4adc5ad6769e4a5a6179dfff271cd4c9dc47a5b
MarcoFalke:
re-ACK b4adc5ad6769e4a5a6179dfff271cd4c9dc47a5b 🏁
Tree-SHA512: 16b59f6ff8140d0229079ca1c6b04f2f4a00a2e49931275150e4f3fe5ac4ec109698b083fa6b223ba9511f328271cc1ab081263669d5da020af7fee83c13e401
fadd73037e266edb844f0972e82e9213171ef214 refactor: Remove implicit-integer-sign-change suppressions in validation.cpp (MarcoFalke)
Pull request description:
A file-wide suppression is problematic because it will wave through future violations, potentially bugs.
Fix that by using per-statement casts.
ACKs for top commit:
shaavan:
ACK fadd73037e266edb844f0972e82e9213171ef214
theStack:
Code-review ACK fadd73037e266edb844f0972e82e9213171ef214
Tree-SHA512: a8a05613be35382b92d7970f958a4e8f4332432056eaa9d72f6719495134b93aaaeea692899d9035654d0e0cf56bcd759671eeeacfd0535582c0ea048ab58a56
4523d28b6bd550bf9f9c724f9aa90c3d2d4ebe3b [test] compare filter and header with the result of the getblockfilter RPC (Niklas Gögge)
3a2464f21619c3065ec2f5d8f7431703c30c964a [rest] drop superfluous rpc serializations flags for block filters (Niklas Gögge)
064abd14a55e0fa1bff530237816a748d01e0ddb [rest] add a more verbose error message for invalid header counts (Niklas Gögge)
83b8f3a8961baa34a136ecfaf62c3ea0d133b6d6 [refactor] various style fix-ups (Niklas Gögge)
Pull request description:
This PR addresses unresolved review comments from [#17631](https://github.com/bitcoin/bitcoin/pull/17631).
This includes:
* various style fix-ups
* returning a more verbose error message for invalid header counts
* removing superfluous rpc serializations flags for block filters
* improving the test to include comparing the block filters returned from the rest with the ones returned from the `getblockfilter` RPC.
ACKs for top commit:
jnewbery:
ACK 4523d28b6bd550bf9f9c724f9aa90c3d2d4ebe3b
brunoerg:
tACK 4523d28b6bd550bf9f9c724f9aa90c3d2d4ebe3b
Tree-SHA512: 634e6b2ae3e1d5f31675a50cfe11a5e74bf5a51b9e7b512d9e18879bf2ed424fc0ac6ec633023f192e3ad12cf0c73b0b51de79dd7ec00844dba3e1493d823b8c
fa1a51cbc1c50a6d3adcad5ccea4c6067f89f7d3 doc: testnet3 was not reset and is doing BIP30 checks again (MarcoFalke)
Pull request description:
ACKs for top commit:
theStack:
ACK fa1a51cbc1c50a6d3adcad5ccea4c6067f89f7d3
Tree-SHA512: 793eccda583a3edb056b142c36a09a5c867f61d90b96e15e6643417d62eb651eb2f3429c5f245bdb062d18ab9bb05b5048c0888aa5a492cb7bb21a2f3f52324e
The logic of these functions is already covered by existing unit tests
using publicly exposed functions of the interface.
Therefore, removing them does not decrease test coverage.
Switches to AddrMan for tests that use no features of AddrManTest.
Also removes unusued AddrManTest variables
Co-Authored-By: Amiti Uttarwar <amiti@uttarwar.org>
fa50d8b66e74792eb46515853e4aa8a99f25561b doc: Remove TODO comment in tx_verify (MarcoFalke)
Pull request description:
The comment has no clear motivation, so it seems better to remove it and fix it when there is a reason.
An alternative (if a fix isn't possible when there is a clear motivation) would be to create an issue thread for easier discussion.
ACKs for top commit:
fanquake:
ACK fa50d8b66e74792eb46515853e4aa8a99f25561b
Tree-SHA512: e9c25bab46a73b7c2db288c62ed9838a5e794b3b72db494173f4502da60b58dec4383064964c0842932cd30e4251fc01ad0c28681e2ef6cb442482eea2bad595
fa993d0e7e4d27ca1cf7d88a5a6ec5b93b7ed503 doc: Fix -changetype help text (MarcoFalke)
Pull request description:
This was forgotten in commit 3ac38058ce0e80a9f4276bfa82951decdb237e9a
ACKs for top commit:
shaavan:
ACK fa993d0e7e4d27ca1cf7d88a5a6ec5b93b7ed503
w0xlt:
ACK fa993d0
josibake:
ACK fa993d0e7e
Tree-SHA512: 9f84b1168e3b3ab06e5c1f4915a1874598b273099eb5878ed28c3a66f1484e34c836fd3c68c4131bee541f3428052f6b66e02b192170752d1082de689d44cd4d
fa562fdd5e36566f8013eee17c6dbf830942094e doc: Remove fixed TODO from wallet/feebumper (MarcoFalke)
Pull request description:
Fixed in commit 9522b53a91f28032c34b94662d50b000534708ce
ACKs for top commit:
shaavan:
ACK fa562fdd5e36566f8013eee17c6dbf830942094e
Tree-SHA512: 968fda0994020c369b7acfc01db109d0f50d4c137fadf533ae55d0e14a353ebbde4320e798cf89e5489f1020c459712631b3967976c1f73d99db8a2d1cbad982
