Age | Commit message (Collapse) | Author |
|
Let GetPathArg method be used more places for path arguments that have
default values, like "-settings" and BITCOIN_SETTINGS_FILENAME in the
next commit.
Also:
- Fix negated argument handling. Return path{} not path{"0"} when path
argument is negated.
- Add new tests for default and negated cases
- Move GetPathArg() method declaration next to GetArg() declarations.
The two methods are close substitutes for each other, so this should
help keep them consistent and make them more discoverable.
|
|
network to CJDNS peers
b7be28cac50046b9f2ddfe63ecafccc80649a36c test: add combined CJDNS/I2P/localhost/onion eviction protection tests (Jon Atack)
0a1bb84770b403ab5cbd9d5474c76f91ce58e8f6 test: add tests for inbound eviction protection of CJDNS peers (Jon Atack)
0c00c0c981fc0b6cec101e68e8c1aeda1ccf33bb test: fix off-by-one logic in an eviction protection test (Jon Atack)
f7b8094d611531c6b41a94715dbc01f56257ccd2 p2p: extend inbound eviction protection by network to CJDNS peers (Jon Atack)
Pull request description:
Extend inbound eviction protection for peers connected over CJDNS, as is the case for peers connected via onion, localhost, and I2P since #21261 and #20197. CJDNS peers seem to have better min ping latency than onion and I2P peers but still higher than that of unencrypted IPv4/6 peers and can be disadvantaged under our eviction criteria. They are also very few in number, which is a further reason to protect them, as the goal of this logic is to favorise the diversity of our peer connections. CJDNS support was added in #23077 for the upcoming v23 release.
ACKs for top commit:
laanwj:
Concept and code review ACK b7be28cac50046b9f2ddfe63ecafccc80649a36c
w0xlt:
tACK b7be28c
Tree-SHA512: 89ebdd217602e16ae14b9bd0d5a25fc09f9b2384c951f820bc0f5a6d8452bbc9042065db817d5d5296c0ad22988491a83fc5b9a611e660c40ebd4f03448c4061
|
|
on non-default ports
36ee76d1afbb278500fc8aa01606ec933b52c17d net: remove unused CNetAddr::GetHash() (Vasil Dimov)
d0abce9a50dd4f507e3a30348eabffb7552471d5 net: include the port when deciding a relay destination (Vasil Dimov)
2e38a0e6865187d1f0d0f016d3df7cce414a7c4f net: add CServiceHash constructor so the caller can provide the salts (Vasil Dimov)
97208634b96f2d9a55f2ead7b0ef407da729d7bd net: open p2p connections to nodes that listen on non-default ports (Vasil Dimov)
Pull request description:
By default, for mainnet, the p2p listening port is 8333. Bitcoin Core
has a strong preference for only connecting to nodes that listen on that
port.
Remove that preference because connections over clearnet that involve
port 8333 make it easy to detect, analyze, block or divert Bitcoin p2p
traffic before the connection is even established (at TCP SYN time).
For further justification see the OP of:
https://github.com/bitcoin/bitcoin/pull/23306
ACKs for top commit:
laanwj:
Concept and light code review ACK 36ee76d1afbb278500fc8aa01606ec933b52c17d
prayank23:
ACK https://github.com/bitcoin/bitcoin/pull/23542/commits/36ee76d1afbb278500fc8aa01606ec933b52c17d
stickies-v:
tACK 36ee76d1a
jonatack:
ACK 36ee76d1afbb278500fc8aa01606ec933b52c17d
glozow:
utACK 36ee76d1afbb278500fc8aa01606ec933b52c17d
Tree-SHA512: 7f45ab7567c51c19fc50fabbaf84f0cc8883a8eef84272b76435c014c31d89144271d70dd387212cc1114213165d76b4d20a5ddb8dbc958fe7e74e6ddbd56d11
|
|
|
|
Doing so can lead to a glibc crash. Also the manpage for fopencookie
warns against this: https://man7.org/linux/man-pages/man3/fopencookie.3.html
|
|
|
|
|
|
fad7ddf9e3710405d727f61d8200d5efed1e705b test: Run symlink regression tests on Windows (MarcoFalke)
Pull request description:
Seems odd to add tests, but not run them on the platform that needs them most.
ACKs for top commit:
laanwj:
Code review ACK fad7ddf9e3710405d727f61d8200d5efed1e705b
ryanofsky:
Code review ACK fad7ddf9e3710405d727f61d8200d5efed1e705b, just removing new test. Would be nice if the test could be added later, of course.
Tree-SHA512: 64b235967a38c2eb90657e8d7a0447bcc8ce81d1b75a275b6c48bd42efd9ea7e7939257e484f297ee84598def3738eaeb289561aeba1dd6a99b258d389995139
|
|
warnings
fafc4eb3637be0a85644c89c355fe68678a62c17 test: Fix Wambiguous-reversed-operator compiler warnings (MarcoFalke)
Pull request description:
Add a missing const to avoid the C++20 clang **compiler warning**:
```
test/fuzz/addrman.cpp:325:22: error: ISO C++20 considers use of overloaded operator '==' (with operand types 'AddrManDeterministic' and 'AddrManDeterministic') to be ambiguous despite there being a unique best viable function [-Werror,-Wambiguous-reversed-operator]
assert(addr_man1 == addr_man2);
~~~~~~~~~ ^ ~~~~~~~~~
/usr/include/assert.h:93:27: note: expanded from macro 'assert'
(static_cast <bool> (expr) \
^~~~
test/fuzz/addrman.cpp:140:10: note: ambiguity is between a regular call to this operator and a call with the argument order reversed
bool operator==(const AddrManDeterministic& other)
^
1 error generated.
```
This patch also fixes the **compile error** if the first operand is `const`:
```
test/fuzz/addrman.cpp:326:23: error: invalid operands to binary expression ('const AddrManDeterministic' and 'AddrManDeterministic')
assert(addr_man_1 == addr_man2);
~~~~~~~~~~ ^ ~~~~~~~~~
/usr/include/assert.h:90:27: note: expanded from macro 'assert'
(static_cast <bool> (expr) \
^~~~
test/fuzz/addrman.cpp:140:10: note: candidate function not viable: 'this' argument has type 'const AddrManDeterministic', but method is not marked const
bool operator==(const AddrManDeterministic& other)
^
1 error generated.
ACKs for top commit:
hebasto:
ACK fafc4eb3637be0a85644c89c355fe68678a62c17, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 92cd62ae06ee1393a6dc2ea6f3f553595a8f8d66f51592d231b42122bfb71ed4801a016daafc85360040339c5ae59b76888265cec37449c4688d6c7768f4567e
|
|
These manual calls to Unload() are no longer necessary because
CBlockIndex's no longer live in the heap as of the previous commit.
|
|
from transifex translator feedback
48742693acc9de837735674057c9aae2fe90bd1d Replace "can not" with "cannot" in docs, user messages, and tests (Jon Atack)
e670edd43441ecb6e5978d65348501c57d856030 User-facing content fixups from transifex translator feedback (Jon Atack)
Pull request description:
Closes #24366.
ACKs for top commit:
laanwj:
Code review re-ACK 48742693acc9de837735674057c9aae2fe90bd1d
hebasto:
re-ACK 48742693acc9de837735674057c9aae2fe90bd1d, only suggested change since my previous [review](https://github.com/bitcoin/bitcoin/pull/24367#pullrequestreview-885938219).
Tree-SHA512: 4dcdcb417251a413e65fab6070515e13a1267c8e0dbcf521386b842511391f24c84a0c2168fe13458c977682034466509bf2a3453719d4d94d3c568fd9f4adb4
|
|
77202f0554dcbbbb167d0ed3927cca0bf4609ce8 [doc] package deduplication (glozow)
d35a3cb3968d7584c7d5c42b121a80f34ea656bf [doc] clarify inaccurate comment about replacements paying higher feerate (glozow)
5ae187f8761f5f85a1ef41d24f75afb7eecf366f [validation] look up transaction by txid (glozow)
Pull request description:
- Use txid, not wtxid, for `mempool.GetIter()`: https://github.com/bitcoin/bitcoin/pull/22674#discussion_r772934994
- Fix a historically inaccurate comment about RBF during the refactors: https://github.com/bitcoin/bitcoin/pull/22855#discussion_r777130441
- Add a section about package deduplication to policy/packages.md: https://github.com/bitcoin/bitcoin/pull/24152#discussion_r802955759 and https://github.com/bitcoin/bitcoin/pull/24152#discussion_r802723149
(I'm intending for this to be in v23 since it's fixups for things that are already merged, which is why I split it from #24152)
ACKs for top commit:
t-bast:
LGTM, ACK https://github.com/bitcoin/bitcoin/pull/24310/commits/77202f0554dcbbbb167d0ed3927cca0bf4609ce8
darosior:
ACK 77202f0554dcbbbb167d0ed3927cca0bf4609ce8
LarryRuane:
ACK 77202f0554dcbbbb167d0ed3927cca0bf4609ce8
Tree-SHA512: a428e791dfa59c359d3ccc67e8d3a4c1239815d2f6b29898e129700079271c00b3a45f091f70b65a6e54aa00a3d5b678b6da29d2a76b6cd6f946eaa7082ea696
|
|
|
|
|
|
|
|
minor bugs
0683f377e1588758da86368f82efee765f89d890 Add tr() descriptor unit tests (Pieter Wuille)
4b2e31a7ae630e68735e9c8e32f1df422ef4aff0 Bugfix: make ToPrivateString work with x-only keys (Pieter Wuille)
18ad54c3b21804ad540631dd4527cbad6d6ccc75 Bugfix: set x-only flag when inferring pk() inside tr() (Pieter Wuille)
Pull request description:
This fixes two bugs in the current logic for `tr()` descriptors:
* ToPrivateString does not always work, because the provided private key may mismatch the parity of the x-only public key.
* The descriptors inferred for `pk()` inside `tr()` have the wrong x-only flag, leading to such descriptors generating the wrong scriptPubKey (roundtripping through ToString does fix it however, so this seems unobservable in the current code).
These were discovered while adding unit tests to descriptor_tests that cover various aspects of `tr()` descriptors, which are now also added here.
ACKs for top commit:
achow101:
ACK 0683f377e1588758da86368f82efee765f89d890
instagibbs:
ACK https://github.com/bitcoin/bitcoin/pull/24343/commits/0683f377e1588758da86368f82efee765f89d890
jonatack:
Code review ACK 0683f377e1588758da86368f82efee765f89d890
Tree-SHA512: fc0e11b45da53054a108effff2029d67b64e508b160a6e22e00c98b506c39ec12ccc95afd21ea68a6c691eb62930afc7af18908f2fa3a954d102afdc67bc355a
|
|
|
|
|
|
|
|
target
fae3f178238df96554dc2495e040f5580b55408a fuzz: Split script formatting from script fuzz target (MarcoFalke)
Pull request description:
This is a follow-up to commit 9237bdaac196951a437accaefa65638149b25978.
The target was improved a bit, but is still taking enormously long. See for example 4096 seconds in https://cirrus-ci.com/task/5153886888525824?logs=ci#L4451.
Most of the time is spent formatting the script. See the flamegraph: 
Thus, I suggest to split up the formatting into a new target. This will:
* Allow more fuzz cycles in the `script` target when exploring the search space with the fuzz engine
* Hopefully allow to reduce the fuzz inputs in `qa-assets` without losing coverage
ACKs for top commit:
fanquake:
ACK fae3f178238df96554dc2495e040f5580b55408a
Tree-SHA512: f86154b23019b7721e5dd10f54d11f4f7603d280471a396cb5256f4c460f48333318a60efe8b77fa8749a4abc67ad2631211b766fde5da70ded9fab8f904747b
|
|
|
|
Work around libstdc++ issue [PR101510] with create_directories where the
leaf already exists as a symlink. Fixes #24257, introduced by the switch
to `std::filesystem`. It is meant to be more thorough than #24266, which
only worked around one instance of the problem.
The issue was fixed upstream in
https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=124eaa50e0a34f5f89572c1aa812c50979da58fc,
but unfortunately we'll have to carry a fix for it for a while.
This introduces a function `fs::create_directories` which wraps
`std::filesystem::create_directories`. This allows easiliy reverting the
workaround when it is no longer necessary.
|
|
dc01cbc538765f64326bca30952c83e3862d0d54 test: Add fs_tests/rename unit test (Hennadii Stepanov)
d4999d40b9bd04dc20111aaaa6ed2d3db1a5caf9 util: Revert back MoveFileExW call for MinGW-w64 (Hennadii Stepanov)
Pull request description:
Unfortunately, bitcoin/bitcoin#24308 introduced a [regression](https://github.com/bitcoin/bitcoin/pull/24308#issuecomment-1037259386) for mingw builds.
The root of the problem is a broken implementation of [`std::filesystem::rename`](https://en.cppreference.com/w/cpp/filesystem/rename). In particular, the expected behavior
> If `old_p` is a non-directory file, then `new_p` must be ... existing non-directory file: `new_p` _is first deleted_...
fails with the "File exists" error.
This PR reverts back the `MoveFileExW` call, and adds the [suggested](https://github.com/bitcoin/bitcoin/pull/24308#pullrequestreview-878832906) unit test.
ACKs for top commit:
vasild:
ACK dc01cbc538765f64326bca30952c83e3862d0d54
Tree-SHA512: c8e5a98844cfa32bec0ad67a1aaa58fe2efd0c5474d3e83490211985b110f83245758a742dcaa0a933a192ab66a7f11807e0c53ae69260b7dd02fc99f6d03849
|
|
This change replaces repetitive code with a helper macro.
|
|
|
|
|
|
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
|
|
lower/higher) start witness programs
34d0e07e929c9dd12727d77896cc47f7ac4be680 Test that OP_1-OP_16 (but not lower/higher) start witness programs (Pieter Wuille)
Pull request description:
Cherry-picks one of the commits adding test coverage from #13062. As [pointed out by aj](https://github.com/bitcoin/bitcoin/pull/13062/files#r492723037):
> could move the test additions to the first commit, since they're testing things that are already true
Pull the additional test code into master earlier.
ACKs for top commit:
laanwj:
Code review ACK 34d0e07e929c9dd12727d77896cc47f7ac4be680
Tree-SHA512: ff0ab2a54613ea6e8246b443363b362dd41b5e464faba4d11be6003aa6588a626cf56e142a3b94465cd37dd3ac4debea08455db96bade336171b6c30ea894950
|
|
GetIter takes a txid, not wtxid.
|
|
|
|
|
|
This new constructor will be useful if we just want to hash a `CService`
object without the two `GetRand()` calls (in `RelayAddress()` in a
subsequent commit).
|
|
By default, for mainnet, the p2p listening port is 8333. Bitcoin Core
has a strong preference for only connecting to nodes that listen on that
port.
Remove that preference because connections over clearnet that involve
port 8333 make it easy to detect, analyze, block or divert Bitcoin p2p
traffic before the connection is even established (at TCP SYN time).
For further justification see the OP of:
https://github.com/bitcoin/bitcoin/pull/23306
|
|
|
|
FormatParagraph
fa2f7d005932bff9b7d27744ae517b9e7910df8d fuzz: Avoid unsigned integer overflow in FormatParagraph (MarcoFalke)
Pull request description:
`FormatParagraph` is only ever called with compile time constant arguments, so I don't see the need for fuzzing it.
Though, keep it for now, but avoid the unsigned integer overflow with this patch.
ACKs for top commit:
laanwj:
Code review ACK fa2f7d005932bff9b7d27744ae517b9e7910df8d
Tree-SHA512: 01fc64a9ef73c183921ca1b0cd8db9514c0a242e3acf215a3393f383ae129e01625ebb16eaf9cb86370eda62d0145c3dcf8f62e40edf5958abc1f777c5687280
|
|
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
|
|
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
Co-authored-by: laanwj <126646+laanwj@users.noreply.github.com>
|
|
fa1b227a727a5056c6fbc7e4f33c19aeb5207718 Remove broken and unused CDataStream methods (MarcoFalke)
faee5f8dc23cd2fcfb6ad62a1d46ad3020ef0c5c test: Create fresh CDataStream each time (MarcoFalke)
fa71114926490e84c9222d315a95684d250e8e34 test: Inline expected_xor (MarcoFalke)
Pull request description:
The `insert` and `erase` methods have many issues:
* They are unused
* They are confusing and hard to read, as they implement "special cases" for optimization, that isn't needed
* They are broken (See https://github.com/bitcoin/bitcoin/pull/24231)
* Fixing them leads to mingw compile errors (See https://github.com/bitcoin/bitcoin/pull/24231#issuecomment-1029286985)
Fix all issues by removing them
ACKs for top commit:
laanwj:
Code review ACK fa1b227a727a5056c6fbc7e4f33c19aeb5207718
Tree-SHA512: 9d9e5d42e6ffc5ae82bdb67cfb5b50b45977ae674acee6ff99092560aebf2fc7e4584ded614e190db0663226fa198e34350517cd7ee57d518de22e9568bc349a
|
|
|
|
|
|
fad81548fa03861c244397201d6b6e6cbf883c38 test: Avoid testing negative block heights (MarcoFalke)
Pull request description:
A negative chain height is only used to denote an empty chain, not the height of any block.
So stop testing that and remove a suppression.
ACKs for top commit:
brunoerg:
crACK fad81548fa03861c244397201d6b6e6cbf883c38
Tree-SHA512: 0f9e91617dfb6ceda99831e6cf4b4bf0d951054957c159b1a05a178ab6090798fae7368edefe12800da24585bcdf7299ec3534f4d3bbf5ce6a6eca74dd3bb766
|
|
d216bc8d76d7f4e9dce58b0bb732a2d4deaf23b6 Re-enable walletinit_verify_walletdir_no_trailing2 test disabled in #20744 (Ryan Ofsky)
80cd64e84296f1166e133c237fa0afc046b01ce2 Re-enable util_datadir check disabled in #20744 (Ryan Ofsky)
Pull request description:
Reenable some broken tests as discussed https://github.com/bitcoin/bitcoin/pull/20744#discussion_r798651736 and https://github.com/bitcoin/bitcoin/pull/20744#discussion_r798678137
Fix windows test cases broken in #20744, by passing normalized path arguments to fs::equivalent, fs::exists, and fs::is_directory, instead of non-normalized arguments. Also re-enable the tests.
It is possible these changes also fix real init behavior on windows when -datadir or -walletdir paths with trailing dots or dashes are used, but it's not clear because I only tested on wine.
ACKs for top commit:
hebasto:
ACK d216bc8d76d7f4e9dce58b0bb732a2d4deaf23b6, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 2099ddfa1a3ad70f7ac2ff413929414a1851d257b280da25c0f5cefb46fd1372b580a1f1ee5280681a1c16e6031f119185cadd4f7a6121298562cf001f711068
|
|
This should also fix an assert error if a -datadir with a trailing slash
is used on windows. This appears to be a real error and regression
introduced with #20744.
On windows (or at least wine), fs calls that actuallly access the
filesystem like fs::equivalent or fs::exists seem to treat directory
paths with trailing slashes as not existing, so it's necessary to
normalize these paths before using them. This fix adds a
path::lexically_normal() call to the failing assert so it passes.
|
|
ef5014d256638735b292672c774446db4003f03b style: wrap long lines in CNode creation and add some comments (Vasil Dimov)
b68349164827f14c472201cad54c4e19a3321261 scripted-diff: rename CNode::cs_hSocket to CNode::m_sock_mutex (Vasil Dimov)
c41a1162ac4da437c5d755e8fe2bf636bed22b0f net: use Sock in CNode (Vasil Dimov)
c5dd72e146dd8fa77d29c8689a42322a4d1ec780 fuzz: move FuzzedSock earlier in src/test/fuzz/util.h (Vasil Dimov)
Pull request description:
_This is a piece of #21878, chopped off to ease review._
Change `CNode` to use a pointer to `Sock` instead of a bare `SOCKET`.
This will help mocking / testing / fuzzing more code.
ACKs for top commit:
jonatack:
re-ACK ef5014d256638735b292672c774446db4003f03b changes since last review are the removal of an unneeded dtor and the addition of a style commit
w0xlt:
reACK ef5014d
PastaPastaPasta:
utACK ef5014d256638735b292672c774446db4003f03b, I have reviewed the code, and believe it makes sense to merge
theStack:
Cod-review ACK ef5014d256638735b292672c774446db4003f03b
Tree-SHA512: 7f5414dd339cd2f16f7cbdc5fcec238d68b6d50072934aea10b901f409da28ff1ece6db6e899196616aa8127b8b25ab5b86d000bdcee58b4cadd7a3c1cf560c5
|
|
|
|
Can be reviewed with --ignore-all-space
|
|
|
|
Warning: Replacing fs::system_complete calls with fs::absolute calls
in this commit may cause minor changes in behaviour because fs::absolute
no longer strips trailing slashes; however these changes are believed to
be safe.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
|
|
|
|
faa630aa15bbda0f3b0cf3b6f31cf8fdaeb66975 test: Fix sanitizer suppresions in streams_tests (MarcoFalke)
Pull request description:
Two changes (that also make sense on their own) to remove the file-wide sanitizer suppression:
* `FindByte` no longer takes a `char`, but an `uint8_t`, after commit 196b4599201dbce3e0317e9b98753fa6a244b82d.
* The `key` vector of unsigned chars can be removed and inlined as initializer-list. This avoids a bunch of verbose code like `clear()` and `push_back` of `char`s.
ACKs for top commit:
PastaPastaPasta:
utACK faa630aa15bbda0f3b0cf3b6f31cf8fdaeb66975, I have reviewed the changes and agree it makes sense to merge
Tree-SHA512: 747b9d4676fad6d07f3955668639c93333625e69199ff4c499f01167de3875990d93db85e775a7f5b1b684575dceaec8aa000b4db15525fc47b699bac1c85e3d
|