Do not create strings and compare them to check if one `addr:port`
equals another. Use `CService::operator==()` instead.
`strDefaultProxyGUI` was assigned the same value 3 times. Instead save
it in `const CService ui_proxy` at the beginning of the function.
Both methods do the same thing, so simplify to having just one.
`ToString()` is too generic in this case and it is unclear what it does,
given that there are similar methods:
`ToStringAddr()` (inherited from `CNetAddr`),
`ToStringPort()` and
`ToStringAddrPort()`.
Both methods do the same thing, so simplify to having just one.
Further, `CService` inherits `CNetAddr` and `CService::ToString()`
overrides `CNetAddr::ToString()` but the latter is not virtual which
may be confusing. Avoid such a confusion by not having non-virtual
methods with the same names in inheritance.
"IP" stands for "Internet Protocol".
"IP address" is sometimes shortened to just "IP" or "address".
However, Tor or I2P addresses are not "IP addresses", nor "IPs".
Thus, use "Addr" instead of "IP" for addresses that could be IP, Tor or
I2P addresses:
`CService::ToStringIPPort()` -> `CService::ToStringAddrPort()`
`CNetAddr::ToStringIP()` -> `CNetAddr::ToStringAddr()`
-BEGIN VERIFY SCRIPT-
sed -i 's/ToStringIPPort/ToStringAddrPort/g' -- $(git grep -l ToStringIPPort src)
sed -i 's/ToStringIP/ToStringAddr/g' -- $(git grep -l ToStringIP src)
-END VERIFY SCRIPT-
956c67059c refactor, doc: Improve SetupAddressRelay call in version processing (Martin Zumsande)
3c43d9db1e p2p: Don't self-advertise during VERSION processing (Gleb Naumenko)
Pull request description:
This picks up the last commit from #19843.
Previously, we would prepare to self-announce to a new peer while parsing a `version` message from that peer.
This is redundant, because we do something very similar in `MaybeSendAddr()`, which is called from `SendMessages()` after
the version handshake is finished.
There are a couple of differences:
1) `MaybeSendAddr()` self-advertises to all peers we do address relay with, not just outbound ones.
2) `GetLocalAddrForPeer()` called from `MaybeSendAddr()` makes a probabilistic decision to either advertise what they think we are or what we think we are, while `PushAddress()` on `version` deterministically only does the former if the address from the latter is unroutable.
3) During `version` processing, we haven't received a potential sendaddrv2 message from our peer yet, so self-advertisements with addresses from addrV2-only networks would always be dropped in `PushAddress()`.
Since it's confusing to have two slightly different mechanisms for self-advertising, and the one in `MaybeSendAddr()` is better, remove the one in `version`.
ACKs for top commit:
stratospher:
ACK 956c670
naumenkogs:
ACK 956c67059c
amitiuttarwar:
reACK 956c67059c
Tree-SHA512: 933d40615289f055c022170dde7bad0ac0a1d4be377538bfe9ba64375cfeb03bcd803901591f0739ac4850c880e8475a68fd1ab0330800030ab7f19e38c00274
- Constructors of uint256 to utilize Span instead of requiring a std::vector
- converts m_data into a std::array
- Prefers using `WIDTH` instead of `sizeof(m_data)`
- make all the things constexpr
- replace C style functions with c++ equivalents
- memset -> std::fill
- memcpy -> std::copy
Note: In practice, implementations of std::copy avoid multiple assignments and use bulk copy functions such as std::memmove if the value type is TriviallyCopyable and the iterator types satisfy LegacyContiguousIterator. (https://en.cppreference.com/w/cpp/algorithm/copy)
- memcmp -> std::memcmp
Since seed lines comes with 'str' type, comparing it directly with 0
('int' type) in the if statement was not working at all. This is fixed
by casting 'int' type to the values in the 'good' column of seeds text file.
Lines that starts with comment in the seeds text file are now ignored.
If statement for checking bad seeds are moved to the top of the 'parseline'
function as if seed is bad, there is no point of going forward from there.
8f5c560e11 refactor: Refactored RequestMethodString function to follow developer notes (JoaoAJMatos)
7fd3b9491b refactor: Deleted unreachable code in httpserver.cpp (JoaoAJMatos)
Pull request description:
Some of the code in httpserver.cpp was unreachable, and didn't follow the developer notes.
Continuation of [#26570 ](https://github.com/bitcoin/bitcoin/pull/26570)
ACKs for top commit:
stickies-v:
re-ACK [8f5c560](8f5c560e11)
Tree-SHA512: ba8cf4c6dde9e2bb0ca9d63a0de86dfa37b070803dde71ac8384c261045835697a2335652cf5894511b3af8fd99f30e1cbda4e4234815b8b39538ade90fab3f9
293849a260 univalue: Remove confusing getBool method (Ryan Ofsky)
Pull request description:
Drop `UniValue::getBool` method because it is easy to confuse with the `UniValue::get_bool` method, and could potentially cause bugs. Unlike `get_bool`, `getBool` doesn't ensure that the value is a boolean and returns false for all integer, string, array, and object values instead of throwing an exception.
The `getBool` method is also redundant because it is an alias for `isTrue`. There were only 5 `getBool()` calls in the codebase, so this commit replaces them with `isTrue()` or `get_bool()` calls as appropriate.
These changes were originally made by MarcoFalke in https://github.com/bitcoin/bitcoin/pull/26213 but were dropped to limit the scope of that PR.
ACKs for top commit:
justinpickering:
ACK 293849a260
sipa:
utACK 293849a260
w0xlt:
ACK 293849a260
hebasto:
ACK 293849a260, also verified that the removed `getBool` method is not mentioned in any docs:
furszy:
ACK 293849a2
Tree-SHA512: 9fbfe5e2083410f123b18703a0cc0161ecbbb4958f331c9ff808dcfcc6ad499b0e896abd16fb8ea200c53ba29878db9812ce141e59cc5e0fd174741b0bcb192d
We need to check that the fee is not negative even before it is
finalized. The setting of fees for SFFO may adjust the fee to be
"correct" and no longer negative, but erroneously reduce the amounts too
far. So we need to check this condition before we do those adjustments.
It doesn't make sense to be checking whether the fee paid is underpaying
before we've finished setting the fees. So do that after we have done
the reduction for SFFO and change adjustment for fee overpayment.
The logest running tests should be at the front of the list in
test_runner.py. Since compiling with --enable-debug can have a
significant effect on test runtime, the order is based on the runtime
with that option configured.
test_submit_child_with_parents creates a p2p connection which waits for
the node to announce transactions to it. By whitelisting this
connection, we can reduce the amount of time spent waiting for this
announcement which improves the test runtime and runtime variance.
The work queue exceeded test in interface_rpc.py would repeatedly call
an RPC until the error was achieved. However hitting this error is
dependent on the processing speed of the computer and the optimization
level of the binary. Configurations that result in slower processing
would result in the RPC used being processed before the error could be
hit, resulting the test's runtime having a high variance.
Switching the RPC to waitfornewblock allows it to run in a much more
consistent time that is still fairly fast. waitfornewblock forces
the RPC server to allocate a thread and wait, occupying a spot in the
work queue. This is perfect for this test because the slower the RPC,
the more likely we will achieve the race condition necessary to pass the
test. Using a timeout of 500 ms appears to work reliably without causing
the test to take too long.
The sigops draining script in feature_taproot's block_submit was
initialized with a list that would end up always being iterated by
CScript's constructor. Since this list is very large, a lot of time
would be wasted. By creating and passing a bytes object initialized from
that list, we can avoid this iteration and dramatically improve the
runtime of feature_taproot.
feature_fee_estimation has a lot of loops that hit the RPC many times in
succession in order to setup scenarios. Using batched requests for these
can reduce the test's runtime without effecting the test's behavior.
Generating blocks is slow, especially when --enable-debug. There is no
need to generate a new block for each transaction, so avoid doing that
to improve this test's runtime.
fa7d71accc test: Move rpc_fundrawtransaction.py to wallet_fundrawtransaction.py (MarcoFalke)
fa933d6985 test: Move feature_backwards_compatibility.py to wallet_backwards_compatibility.py (MarcoFalke)
Pull request description:
The tests only tests the wallet and it doesn't make sense to extend it for other stuff, so clarify that.
ACKs for top commit:
fanquake:
ACK fa7d71accc
pablomartin4btc:
re-ACK fa7d71accc
Tree-SHA512: 9dc131ed8ff119bd6d43d04ecc5c96d02f2d6fdc3d0805492774a414c0fbe6a984da7631154122a080c54ddd25fc5a5bdd52b282c918fce4932057ba72c2bf71
Replacing `install` with `install-lib` and `install-bin` is not strictly
necessary just to update the library, but it takes advantage of recent
changes in the new version, and makes the build more minimal.
Drop UniValue::getBool method because it is easy to confuse with the
UniValue::get_bool method, and could potentially cause bugs. Unlike get_bool,
getBool doesn't ensure that the value is a boolean and returns false for all
integer, string, array, and object values instead of throwing an exceptions.
The getBool method is also redundant because it is an alias for isTrue. There
were only 5 getBool() calls in the codebase, so this commit replaces them with
isTrue() or get_bool() calls as appropriate.
These changes were originally made by MarcoFalke in
https://github.com/bitcoin/bitcoin/pull/26213 but were dropped to limit the
scope of that PR.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
fabb24cbef test: Use last release in compatibility tests (MarcoFalke)
Pull request description:
In compatibility tests it makes sense to always use the last release without the new feature, as it is likely more in use than any even older previous release.
ACKs for top commit:
Sjors:
utACK fabb24c
Tree-SHA512: beb854f4d28ba313282e1e0303abb0e09377828b138bde5a3e209337210b6b4c24855ab90a68f8789387001e4ca33b15cc37dbc9b7809929f4e7d1b69833a527
affbf58a1e build: Move environment variables into `$(package)_config_env` (Hennadii Stepanov)
d44fcd3c97 build: Make $(package)_*_env available to all $(package)_*_cmds (Hennadii Stepanov)
Pull request description:
On master (1e7564eca8) the depends build system, which is based on pure GNU Make, works, but it lacks robustness, and in some corner cases it fails. For example, see bitcoin/bitcoin#22552.
Another [bug](https://github.com/bitcoin/bitcoin/issues/22719) in the depends build system has already become a problem at least two times in the past (https://github.com/bitcoin/bitcoin/pull/16883#issuecomment-683817472 and https://github.com/bitcoin/bitcoin/pull/24134). Each time the problem was solved with other means.
The initial [solution](https://github.com/bitcoin/bitcoin/pull/19882) had some discussion. Also it was discussed on the IRC meeting in #bitcoin-core-builds channel. This PR, actually, is a resurrection of it, as the bug silently struck pretty [recently](https://github.com/bitcoin/bitcoin/pull/24134).
The bug is well described in bitcoin/bitcoin#22719.
Here is another, a bit simpler description, which requires only basic shell (bash, dash etc) experience.
After creating targets by this code:1e7564eca8/depends/funcs.mk (L280) a "draft" line of recipe like `$($(1)_config_env) $(call $(1)_config_cmds, $(1))` becomes a shell command sequence `VAR1=foo VAR2=bar command1 && command2` which is supposed to be executed in a [new sub-shell](https://www.gnu.org/software/make/manual/html_node/Execution.html#Execution).
Please note that `VAR1=foo VAR2=bar` part is visible for the first `command1` only (for details see shell docs). Example:
```sh
$ VAR1="foo" VAR2="bar" echo "begin" && printenv VAR1 && printenv VAR2 && echo "end"
begin
$ echo $?
1
```
Using the `export` command is a trivial solution:
```sh
$ export VAR1="foo" VAR2="bar"; echo "begin" && printenv VAR1 && printenv VAR2 && echo "end"
begin
foo
bar
end
$ echo $?
0
```
As a [new sub-shell](https://www.gnu.org/software/make/manual/html_node/Execution.html#Execution) is invoked for each line of the recipe, there are no side effects of using `export`. It means this solution should not be considered invasive.
Fixes bitcoin/bitcoin#22719.
---
Also this PR removes no longer needed crutch from `qt.mk`.
ACKs for top commit:
fanquake:
ACK affbf58a1e
Tree-SHA512: 0ce2cf82870a7774bdf1592fac50857126ae47da902e349f1092d50109223be9d6a8efd5e92ec08c2ca775b17516482aabaf232378950ade36484a883acc177b
fa579f3063 refactor: Pass reference to last header, not pointer (MacroFake)
Pull request description:
It is never a nullptr, otherwise an assertion would fire in UpdatePeerStateForReceivedHeaders.
Passing a reference makes the code easier to read and less brittle.
ACKs for top commit:
john-moffett:
ACK fa579f3
aureleoules:
ACK fa579f3063
Tree-SHA512: 9725195663a31df57ae46bb7b11211cc4963a8f3d100f60332bfd4a3f3327a73ac978b3172e3007793cfc508dfc7c3a81aab57a275a6963a5ab662ce85743fd0
07dfbb5bb8 Make static nLastFlush and nLastWrite Chainstate members (Aurèle Oulès)
Pull request description:
Fixes #22189.
The `static std::multimap<uint256, FlatFilePos> mapBlocksUnknownParent; ` referenced in the issue was already fixed by #25571. I don't believe Chainstate references any other static variables.
ACKs for top commit:
jamesob:
ACK 07dfbb5bb8 ([`jamesob/ackr/26513.1.aureleoules.make_static_nlastflush_a`](https://github.com/jamesob/bitcoin/tree/ackr/26513.1.aureleoules.make_static_nlastflush_a))
theStack:
Concept and code-review ACK 07dfbb5bb8
Tree-SHA512: 0f26463c079bbc5e0e62707d4ca4c8c9bbb99edfa3391d48d4915d24e2a1190873ecd4f9f11da25b44527671cdc82c41fd8234d56a4592a246989448d34406b0
