105941b726 net: use stronger AddLocal() for our I2P address (Vasil Dimov)
Pull request description:
There are two issues:
### 1. Our I2P address not added to local addresses.
* `externalip=` is used with an IPv4 address (this sets automatically `discover=0`)
* No `discover=1` is used
* `i2psam=` is used
* No `externalip=` is used for our I2P address
* `listenonion=1 torcontrol=` are used
In this case `AddLocal(LOCAL_MANUAL)` [is used](94f83534e4/src/torcontrol.cpp (L354)) for our `.onion` address and `AddLocal(LOCAL_BIND)` [for our](94f83534e4/src/net.cpp (L2247)) `.b32.i2p` address, the latter being [ignored](94f83534e4/src/net.cpp (L232-L233)) due to `discover=0`.
### 2. Our I2P address removed from local addresses even if specified with `externalip=` on I2P proxy restart.
* `externalip=` is used with our I2P address (this sets automatically `discover=0`)
* No `discover=1` is used
* `i2psam=` is used
In this case, initially `externalip=` causes our I2P address to be [added](94f83534e4/src/init.cpp (L1266)) with `AddLocal(LOCAL_MANUAL)` which overrides `discover=0` and works as expected. However, if later the I2P proxy is shut down [we do](94f83534e4/src/net.cpp (L2234)) `RemoveLocal()` in order to stop advertising our I2P address (since we have lost I2P connectivity). When the I2P proxy is started and we reconnect to it, restoring the I2P connectivity, [we do](94f83534e4/src/net.cpp (L2247)) `AddLocal(LOCAL_BIND)` which does nothing due to `discover=0`.
To resolve those two issues, use `AddLocal(LOCAL_MANUAL)` for I2P which is also what we do with Tor.
ACKs for top commit:
laanwj:
Code review ACK 105941b726
Tree-SHA512: 0c9daf6116b8d9c34ad7e6e9bbff6e8106e94e4394a815d7ae19287aea22a8c7c4e093c8dd8c58a4a1b1412b2575a9b42b8a93672c8d17f11c24508c534506c7
29c9e2c2d2 wallet: Do not iterate a directory if having an error while accessing it (Hennadii Stepanov)
Pull request description:
On Windows when `ListDatabases` tries to iterate any system folder, e.g., "System Volume Information", it falls into an infinite loop.
This PR fixes this bug. Now the `debug.log` contains:
```
2021-05-12T09:07:53Z ListDatabases: Access is denied D:/System Volume Information -- skipping.
```
An easy way to reproduce the bug and test this PR is to pass the `-walletdir=D:\` command-line option, and run the `listwalletdir` RPC, or File -> Open Wallet in the GUI menu.
Fixes#20081.
Fixes#21136.
Fixes#21904.
Also https://bitcoin.stackexchange.com/questions/99243/listwalletdir-access-is-denied-d-system-volume-information
ACKs for top commit:
prayank23:
ACK 29c9e2c2d2
promag:
Code review ACK 29c9e2c2d2.
meshcollider:
Code review ACK 29c9e2c2d2
Tree-SHA512: b851c88e6d09626f4cb81acc2fa59a563b2aee64582963285715bf785c64b872e8bf738aa6b27bdbaf4c3e5c8565c2dc2c802135f9aa1f48b4b913435bc5d793
c30dd02cd8 refactor: remove redundant fOnlySafe argument (t-bast)
Pull request description:
The `fOnlySafe` argument to `AvailableCoins` is now redundant, since #21359 added a similar field inside the `CCoinControl` struct (see https://github.com/bitcoin/bitcoin/pull/21359#discussion_r591578684).
Not all code paths create a `CCoinControl` instance, but when it's missing we can default to using only safe inputs which is backwards-compatible.
ACKs for top commit:
instagibbs:
utACK c30dd02cd8
promag:
Code review ACK c30dd02cd8.
achow101:
ACK c30dd02cd8
meshcollider:
Code review + test run ACK c30dd02cd8
Tree-SHA512: af3cb598d06f233fc48a7c9c45bb14da92b5cf4168b8dbd4f134dc3e0c2b615c6590238ddb1eaf380aea5bbdd3386d2ac8ecd7d22dfc93579adc39248542839b
fa340b8794 refactor: Avoid magic value of all-zeros in assumeutxo base_blockhash (MarcoFalke)
fae33f98e6 Fix assumeutxo crash due to invalid base_blockhash (MarcoFalke)
fa5668bfb3 refactor: Use type-safe assumeutxo hash (MarcoFalke)
0000007709 refactor: Remove unused code (MarcoFalke)
faa921f787 move-only: Add util/hash_type (MarcoFalke)
Pull request description:
Starting with commit d6af06d68a, a block hash of all-zeros is invalid and will lead to a crash of the node. Can be tested by cherry-picking the test changes without the other changes.
Stack trace (copied from https://github.com/bitcoin/bitcoin/pull/21584#discussion_r612673879):
```
#0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1 0x00007ffff583c8b1 in __GI_abort () at abort.c:79
#2 0x00007ffff582c42a in __assert_fail_base (fmt=0x7ffff59b3a38 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n",
assertion=assertion@entry=0x555556c8b450 "!hashBlock.IsNull()", file=file@entry=0x555556c8b464 "txdb.cpp", line=line@entry=89,
function=function@entry=0x555556c8b46d "virtual bool CCoinsViewDB::BatchWrite(CCoinsMap &, const uint256 &)") at assert.c:92
#3 0x00007ffff582c4a2 in __GI___assert_fail (assertion=0x555556c8b450 "!hashBlock.IsNull()", file=0x555556c8b464 "txdb.cpp", line=89,
function=0x555556c8b46d "virtual bool CCoinsViewDB::BatchWrite(CCoinsMap &, const uint256 &)") at assert.c:101
#4 0x000055555636738b in CCoinsViewDB::BatchWrite (this=0x5555577975c0, mapCoins=std::unordered_map with 110 elements = {...}, hashBlock=...) at txdb.cpp:89
#5 0x00005555564a2e80 in CCoinsViewBacked::BatchWrite (this=0x5555577975f8, mapCoins=std::unordered_map with 110 elements = {...}, hashBlock=...) at coins.cpp:30
#6 0x00005555564a43de in CCoinsViewCache::Flush (this=0x55555778eaf0) at coins.cpp:223
#7 0x00005555563fc11d in ChainstateManager::PopulateAndValidateSnapshot (this=0x55555740b038 <g_chainman>, snapshot_chainstate=..., coins_file=..., metadata=...)
at validation.cpp:5422
#8 0x00005555563fab3d in ChainstateManager::ActivateSnapshot (this=0x55555740b038 <g_chainman>, coins_file=..., metadata=..., in_memory=true) at validation.cpp:5299
#9 0x0000555555e8c893 in validation_chainstatemanager_tests::CreateAndActivateUTXOSnapshot<validation_chainstatemanager_tests::chainstatemanager_activate_snapshot::test_method()::$_12>(NodeContext&, boost::filesystem::path, validation_chainstatemanager_tests::chainstatemanager_activate_snapshot::test_method()::$_12) (node=...,
root=..., malleation=...) at test/validation_chainstatemanager_tests.cpp:199
#10 0x0000555555e8877a in validation_chainstatemanager_tests::chainstatemanager_activate_snapshot::test_method (this=0x7fffffffc8d0)
at test/validation_chainstatemanager_tests.cpp:262
ACKs for top commit:
laanwj:
Code review re-ACK fa340b8794
jamesob:
ACK fa340b8794 ([`jamesob/ackr/21584.1.MarcoFalke.fix_assumeutxo_crash_due`](https://github.com/jamesob/bitcoin/tree/ackr/21584.1.MarcoFalke.fix_assumeutxo_crash_due))
Tree-SHA512: c2c4e66c1abfd400ef18a04f22fec1f302f1ff4d27a18050f492f688319deb4ccdd165ff792eee0a1f816e7b69fb64080662b79517ab669e3d26b9eb77802851
bc4538806e build: add *~ to .gitignore (Sjors Provoost)
Pull request description:
The file `configure~` recently started appearing for me on macOS (11.3.1) whenever configure is (re)run.
ACKs for top commit:
hebasto:
ACK bc4538806e, tested on Linux Mint 20.1 with different build scenarios including cross-compiling for Windows and macOS.
Tree-SHA512: 830c7baf392ff6d66250a79c6ed0a98dac3daaace54a6d2e7940b9a72e3bac79ab44bbecd7642c931fde8a446654e2260d6afdecc679a1743fae6ec5eeda79f1
Homebrew autoconf version 2.7.1 introduces configure~ as a build artifact.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
9c891b64ff net: initialize nMessageSize to max uint32_t instead of -1 (eugene)
Pull request description:
nMessageSize is uint32_t and is set to -1. This will warn with `-fsanitize=implicit-integer-sign-change` when V1TransportDeserializer calls into the ctor. This pull initializes nMessageSize to `numeric_limits<uint32_t>::max()` instead and removes the ubsan suppression.
ACKs for top commit:
laanwj:
Code review ACK 9c891b64ff
promag:
Code review ACK 9c891b64ff.
Tree-SHA512: f05173d9553a01d207a5a7f8ff113d9e11354c50b494a67d44d3931c151581599a9da4e28f40edd113f4698ea9115e6092b2a5b7329c841426726772076c1493
- drop redundant PF_ permission flags prefixes
- drop ALL_CAPS naming per https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Renum-caps
- rename IsImplicit to Implicit
-BEGIN VERIFY SCRIPT-
s() { git grep -l "$1" src | xargs sed -i "s/$1/$2/g"; }
s 'PF_NONE' 'None'
s 'PF_BLOOMFILTER' 'BloomFilter'
s 'PF_RELAY' 'Relay'
s 'PF_FORCERELAY' 'ForceRelay'
s 'PF_DOWNLOAD' 'Download'
s 'PF_NOBAN' 'NoBan'
s 'PF_MEMPOOL' 'Mempool'
s 'PF_ADDR' 'Addr'
s 'PF_ISIMPLICIT' 'Implicit'
s 'PF_ALL' 'All'
-END VERIFY SCRIPT-
faad68fcd4 index: Avoid async shutdown on init error (MarcoFalke)
Pull request description:
An async shutdown during init is confusing when a simple boolean return value can be used for a synchronous shutdown.
This also changes the error message on stderr from:
```
Error: A fatal internal error occurred, see debug.log for details
Error: A fatal internal error occurred, see debug.log for details
```
To:
```
Error: basic block filter index best block of the index goes beyond pruned data. Please disable the index or reindex (which will download the whole blockchain again)
ACKs for top commit:
laanwj:
Code review ACK faad68fcd4
Tree-SHA512: 92dd895266d6d15a6b1a5c081c9b83f83d5c82e9bfceb3ea0664f48540812239e274c829ff0271c4a0afb6d6a8f67d89c5af20d719982ad62999a41ca0623274
fa74bfc860 fuzz: Run const CScript member functions only once (MarcoFalke)
Pull request description:
Those functions should be O(N) in the input size (or maybe worse, I didn't check), so if the fuzz input dictates to run them N times, the complexity is N^2.
Fix this by calling them only once.
Can be reviewed with: `--ignore-all-space --word-diff-regex=.`
Input: https://github.com/bitcoin/bitcoin/files/6464685/clusterfuzz-testcase-minimized-input.log
Hopefully fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34101
ACKs for top commit:
practicalswift:
cr ACK fa74bfc860: patch looks correct, rationale makes sense and patch touches only `src/test/fuzz/`
Tree-SHA512: d579f7a2103ec154bf482a872142e55a1d9e7673d33a22a4c4230186fdd1b6618846463f4e25941031cc8c4bd1ea8d06cb49ae1bb1ec4af115497f5e5de1e19c
d420e5c1c0 guix-attest: Avoid incomplete sigdirs with ERR traps (Carl Dong)
feda2c8e31 guix: Skip attesting to dist-archive (Carl Dong)
d522d8006b guix: Attest to inputs in inputs.SHA256SUMS (Carl Dong)
f9e2960c01 guix: Construct $OUTDIR in ${DISTSRC}/output (Carl Dong)
022abc85fc guix: Minor quoting fix in libexec/build.sh (Carl Dong)
c83c4fa5b7 guix-attest: Allow skipping GPG signing with NO_SIGN (Carl Dong)
0e1c2e448c guix-attest: Use ascii-armor signatures (Carl Dong)
b5fd89c4c8 guix-attest: Only use cross-platform flags for find+xargs (Carl Dong)
5926432ba6 guix: Add guix-verify script (Carl Dong)
30daf76a97 guix: Add guix-attest script (Carl Dong)
Pull request description:
Adds replacements for `gsign` and `gverify`.
Personally I'm not a big fan of using the word "sign" as it's been used to refer to both codesigning and GPG signing.
ACKs for top commit:
laanwj:
Code review and tested ACK d420e5c1c0
Tree-SHA512: 93d82d201f4596eaea0e3825aa55b013dfb91790e6ccee79893833d37921513d7b4e735f0641103e1e2ea8308abe4cb6218b73160924708802f2e0e3f7f6caf1
-BEGIN VERIFY SCRIPT-
s() { git grep -l "$1" -- 'src' ':!src/net_permissions.h' | xargs sed -i -E "s/([^:])$1/\1NetPermissionFlags::$1/"; }
s 'PF_NONE'
s 'PF_BLOOMFILTER'
s 'PF_RELAY'
s 'PF_FORCERELAY'
s 'PF_DOWNLOAD'
s 'PF_NOBAN'
s 'PF_MEMPOOL'
s 'PF_ADDR'
s 'PF_ISIMPLICIT'
s 'PF_ALL'
-END VERIFY SCRIPT-
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
792be53d3e refactor: Replace std::bind with lambdas (Hennadii Stepanov)
a508f718f3 refactor: Use appropriate thread constructor (Hennadii Stepanov)
30e4448215 refactor: Make TraceThread a non-template free function (Hennadii Stepanov)
Pull request description:
This PR does not change behavior.
Its goal is to improve readability and maintainability of the code.
ACKs for top commit:
jnewbery:
utACK 792be53d3e
jonatack:
tACK 792be53d3e
MarcoFalke:
cr ACK 792be53d3e
Tree-SHA512: a03142f04f370f6bc02bd3ddfa870819b51740fcd028772241d68c84087f95a2d78207cbd5edb3f7c636fcf2d76192d9c59873f8f0af451d3b05c0cf9cf234df
b95f7f8ac0 build, qt, refactor: Drop sed commands for win32-g++/qmake.conf (Hennadii Stepanov)
Pull request description:
Such possibility is [available](https://codereview.qt-project.org/c/qt/qtbase/+/165348) since Qt 5.8.0.
ACKs for top commit:
fanquake:
ACK b95f7f8ac0
Tree-SHA512: e56a3d208a6bd5d42c722f8b344010fe7d1b6f7a28486613dfcb03f0403a47cee8476e2366eeaac401a19836cd09f782e8741a1e781ab4d78f72c500a30e4929
99993f0664 fuzz: Avoid excessively large min fee rate in tx_pool (MarcoFalke)
Pull request description:
Any fee rate above 1 BTC / kvB is clearly nonsense, so no need to fuzz this.
Hopefully fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34078
ACKs for top commit:
practicalswift:
cr ACK 99993f0664: patch looks correct despite no `fa` prefix in commit hash
Tree-SHA512: bd3651d354b13d889ad1708d2b385ad0479de036de74a237346eefad5dbfb1df76ec02b55ec00487ec598657ef6102f992302b14c4e47f913a9962f81f4157e6
fa95555a49 fuzz: Limit max insertions in timedata fuzz test (MarcoFalke)
Pull request description:
It is debatable whether a size of the median filter other than `200` (the only size used in production) should be fuzzed. For now add a minimal patch to cap the max insertions. Otherwise the complexity is N^2 log(N), where N is the size of the fuzz input.
Hopefully fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34167
ACKs for top commit:
practicalswift:
cr ACK fa95555a49: patch looks correct
Tree-SHA512: be7737e9f4c906053e355641de84dde31fed37ed6be4c5e92e602ca7675dffdaf06b7063b9235ef541b05d3d5fd689c99479317473bb15cb5271b8baabffd0f2
36fb036d25 p2p: allow NetPermissions::ClearFlag() only with PF_ISIMPLICIT (Jon Atack)
4e0d5788ba test: add net permissions noban/download unit test coverage (Jon Atack)
dde69f20a0 p2p, bugfix: use NetPermissions::HasFlag() in CConnman::Bind() (Jon Atack)
Pull request description:
This is a bugfix follow-up to #16248 and #19191 that was noticed in #21506. Both v0.21 and master are affected.
Since #19191, noban is a multi-flag that implies download, so the conditional in `CConnman::Bind()` using a bitwise AND on noban will return the same result for both the noban status and the download status. This means that download peers are incorrectly not being added to local addresses because they are mistakenly seen as noban peers.
The second commit adds unit test coverage to illustrate and test the noban/download relationship and the `NetPermissions` operations involving them.
The final commit adds documentation and disallows calling `NetPermissions::ClearFlag()` with any second param other than `NetPermissionFlags` "implicit" -- per current usage in the codebase -- because `ClearFlag()` should not be called with any second param that is a subflag of a multiflag, e.g. "relay" or "download," as that would leave the result in an invalid state corresponding to none of the existing NetPermissionFlags. Thanks to Vasil Dimov for noticing this.
ACKs for top commit:
theStack:
re-ACK 36fb036d25☕
vasild:
ACK 36fb036d25
hebasto:
ACK 36fb036d25, I have reviewed the code and it looks OK, I agree it can be merged.
kallewoof:
Code review ACK 36fb036d25
Tree-SHA512: 5fbc7ddbf31d06b35bf238f4d77ef311e6b6ef2e1bb9893f32f889c1a0f65774a3710dcb21d94317fe6166df9334a9f2d42630809e7fe8cbd797dd6f6fc49491
There are two issues:
1. Our I2P address not added to local addresses.
* `externalip=` is used with an IPv4 address (this sets automatically
`discover=0`)
* No `discover=1` is used
* `i2psam=` is used
* No `externalip=` is used for our I2P address
* `listenonion=1 torcontrol=` are used
In this case `AddLocal(LOCAL_MANUAL)` is used for our `.onion` address
and `AddLocal(LOCAL_BIND)` for our `.b32.i2p` address, the latter being
ignored due to `discover=0`.
2. Our I2P address removed from local addresses even if specified
with `externalip=` on I2P proxy restart.
* `externalip=` is used with our I2P address (this sets automatically
`discover=0`)
* No `discover=1` is used
* `i2psam=` is used
In this case, initially `externalip=` causes our I2P address to be added
with `AddLocal(LOCAL_MANUAL)` which overrides `discover=0` and works as
expected. However, if later the I2P proxy is shut down we do
`RemoveLocal()` in order to stop advertising our I2P address (since we
have lost I2P connectivity). When the I2P proxy is started and we
reconnect to it, restoring the I2P connectivity, we do
`AddLocal(LOCAL_BIND)` which does nothing due to `discover=0`.
To resolve those two issues, use `AddLocal(LOCAL_MANUAL)` for I2P which
is also what we do with Tor.
fae196147b doc: Clarify that feerates are per virtual size (MarcoFalke)
fa83e95ac6 scripted-diff: Clarify that feerates are per virtual size (MarcoFalke)
Pull request description:
By implementing segwit, it is already clear that all feerates in Bitcoin Core are denoted in (amount/virtual size). Though, there is inconsistency, as some places use kvB, some use kB. Thus, replace all with "kvB".
See also commit 6da3afbaee, which did the replacement for wallet RPCs.
ACKs for top commit:
ryanofsky:
Code review ACK fae196147b. Checked instances where units were being added in the second commit and they all looked right.
Tree-SHA512: ab70d13cde7d55c1ac931bddc2b45aa218fc75ef46cb6ea9e5a30b1d4dbf27889c2b6357299a6c5427912443a46ec3592a4809dae335e03162bd2120a0f7f8ad
