fa5af94de6 ci: Run base install at most once (MarcoFalke)
Pull request description:
This should avoid errors when running it twice. For example, network errors on the second invocation of 'apt update'; or unguarded modifications such as APPEND_APT_SOURCES_LIST, which will append the same string repeatedly.
The base install may be run twice in Cirrus CI with dockerfiles, or locally when running twice with DANGER_RUN_CI_ON_HOST specified.
ACKs for top commit:
josibake:
utACK fa5af94de6
Tree-SHA512: 020ba747e282076ddf10547025bb576ce626378d9f8313e09b1a5f99af1cf8237e44957aa4a0b8d64015bc730a090c83af0b3908a8eda15d3fb1ca7dae69dd7c
This should avoid errors when running it twice. For example, network
errors on the second invocation of 'apt update'; or unguarded
modifications such as APPEND_APT_SOURCES_LIST, which will append the
same string repeatedly.
The base install may be run twice in Cirrus CI with dockerfiles, or
locally when running twice with DANGER_RUN_CI_ON_HOST specified.
ed4a8339b8 ci: fix git dubious permissions error (josibake)
Pull request description:
fixes https://github.com/bitcoin/bitcoin/pull/27376#issuecomment-1496449588
this appears to be caused by a more recent version of git being sensitive to mismatched permissions on directories. we didn't notice this before because we were using two separate user accounts to fix up dir permissions in the container , but the second account was removed in #27376
there might be a more elegant way to do this, but this does the trick and seems to be the way others are fixing this issue around the internets.
ACKs for top commit:
RandyMcMillan:
ACK ed4a833
hebasto:
re-ACK ed4a8339b8
Tree-SHA512: dad467deca101a24f3ed34b3e26a1db5099a5bd5c3e9c9a22771c59848f7d7e7843c7386348e6fdf86d5a556e4706e5e20005d7a6637193e1c8aef7a5ff7fb19
6a9a4d13b2 Fixes compile errors in MSVC build #27332 (Ethan Heilman)
Pull request description:
This PR is designed to address the issue https://github.com/bitcoin/bitcoin/issues/27332. The MSVC build is failing because of two bugs in how the build is configured.
The issue
====
When running `msbuild build_msvc\bitcoin.sln -property:Configuration=Release -maxCpuCount -verbosity:minima`l the build fails with following two errors.
* `C:\Users\e0\Documents\GitHub\bitcoin\src\httpserver.cpp(637,9): error C2664: 'void evhttp_connection_get_peer(evhttp_connection *,const char **,uint16_t *)': cannot convert argument 2 from 'char **' to 'const char **' [C:\Users\e0\Documents\GitHub\bitcoin\build_msvc\libbitcoin_node\libbitcoin_node .vcxproj]`
This error is occurs because bitcoin is using the wrong function signature for `evhttp_connection_get_peer` in libevent. In automake builds, configure.ac inspects the version of libevent it is building against and then defines `HAVE_EVHTTP_CONNECTION_GET_PEER_CONST_CHAR` to flag the source code to use the correct signature. In MSVC build there does not appear to be a mechanism to do this. So it uses the wrong signature and fails. See the PR https://github.com/bitcoin/bitcoin/pull/23607 for when this logic was added to automake builds.
* `event.lib(evutil_rand.c.obj) : error LNK2019: unresolved external symbol BCryptGenRandom referenced in function arc4_seed [C:\Users\e0\Documents\GitHub\bitcoin\build_msvc\bitcoin-cli\bitcoin-cli.vcxproj]
C:\Users\e0\Documents\GitHub\bitcoin\build_msvc\x64\Release\bitcoin-cli.exe : fatal error LNK1120: 1 unresolved externals [C:\Users\e0\Documents\GitHub\bitcoin\build_msvc\bitcoin-cli\bitcoin-cli.vcxproj]`
This error is caused by msbuild not being able to find the library bcrypt.lib because it has not been configured to use bcrypt.lib.
Fixes
====
While for automake builds a macro is being define to configure the current function signature for `evhttp_connection_get_peer` in libevent, this macro is not being defined for MSVC builds.
1. This PR addresses this issue by assuming more recent version of libevent is installed and always defining `HAVE_EVHTTP_CONNECTION_GET_PEER_CONST_CHAR`. This logic is more brittle the automake logic, but someone following the MSVC build instructions should only get the more recent version of libevent.
2. This PR fixes the bcrypt.lib errors this by setting this library as a dependency in build_msvc/common.init.vcxproj.in.
ACKs for top commit:
hebasto:
re-ACK 6a9a4d13b2
Tree-SHA512: 69d2cb6a62ecca976540a39e5f83a4e5386d920a564761cedc5127d82e5fa66ced7fcde3e5e5eb3bd6cde1cc78f843e94aa2789f02bd3e3414118030017d0387
Uses a min-effective-value heap, so we can remove the least valuable input/s
while the selected weight exceeds the maximum allowed weight.
Co-authored-by: Murch <murch@murch.one>
The simplest scenario where this is useful is on the 'check_max_weight' unit test
already:
We create 1515 UTXOs with 0.033 BTC each, and 1 UTXO with 50 BTC. Then perform
Coin Selection.
As the selection of the 1515 small UTXOs exceeds the max allowed tx size, the
expectation here is to receive a selection result that only contain the big
UTXO (which is not happening for the reasons stated below).
As knapsack returns a result that exceeds the max allowed transaction size, we
fallback to SRD, which selects coins randomly up until the target is met. So
we end up with a selection result with lot more coins than what is needed.
436df1e826 depends: add NO_HARDEN option (fanquake)
Pull request description:
Add an option that when passed, will disable hardening options, and pass `--disable-hardening` through to configure. Due to the way we link `libssp` for Windows builds, they now fail (after #27118), if building with depends, and configuring with `--disable-hardening` (Windows is the odd build out here). See: https://github.com/bitcoin/bitcoin/pull/27118#issuecomment-1492606272.
This change would add a depends option such that, if someone wants to build with depends, for Windows, without hardening, they can do so. This may also be useful when building for debugging.
ACKs for top commit:
hebasto:
re-ACK 436df1e826
Tree-SHA512: 5a3ef5ec87b10a5ad0a284201988ce94789451735c7c7e20d337f7232955b0b9a0addab1c3b5725755f00d8ce6741aa9c8cb5e3d48d926515b7dde46acdbcaa0
fa584b4d01 test: Remove windows workaround in authproxy (MarcoFalke)
Pull request description:
I wonder if the windows issues have also been fixed by bumping the server timeout in commit 88134fcee9.
I guess the only way to find out and try.
Note that even with the workaround, the issue would still happen occasionally: https://github.com/bitcoin/bitcoin/issues/18623
ACKs for top commit:
fanquake:
ACK fa584b4d01
Tree-SHA512: 1c633d7d8b54fa47bb851457d48b7d80bf6d6ea353db80610231e0bd385061b795f97ea2b64022fa24e79b4d7dcb5e947e0d637da228f10998e950d84b920878
73f4eb511c Check that the Timestamp String is valid (John Moffett)
Pull request description:
Follow-up to https://github.com/bitcoin/bitcoin/pull/27233
The current `FormatISO8601DateTime` function will return an empty string if it encounters an error when converting the `int64_t` seconds-since-epoch to a formatted date time. In the unlikely case that happens, here `strStamped.pop_back()` would be undefined behavior.
ACKs for top commit:
MarcoFalke:
lgtm ACK 73f4eb511c
stickies-v:
ACK 73f4eb511c
Tree-SHA512: 089ed639c193deb98870a8385039b31b4baed821ea907937bfc6f65a5b0981bbf8284b2afec81b2d0a922e2340716b48cf55349640eb6b8c311ef7af25abc361
9fbc5fcd28 build: remove ancient unused define (Cory Fields)
Pull request description:
The generic define was removed in [upstream miniupnpc in 2014](f6774e3316).
Noticed while reviewing hebasto's new CMake buildsystem: https://github.com/hebasto/bitcoin/pull/12#discussion_r1156267350.
ACKs for top commit:
RandyMcMillan:
utACK 9fbc5fc
hebasto:
ACK 9fbc5fcd28
TheCharlatan:
ACK 9fbc5fcd28
Tree-SHA512: 2075a2e63441e32d939c23f30f811c73dd1ab1ddcc9e6bf079cc8060e77d9a2a6d707add7b72e9251939a53b53e28a134e9bb330ccf3556244497728f1b091ae
54e4061189 refactor: don't avoid sys/types.h on when building for Windows (fanquake)
Pull request description:
This is the only place these defines are used. They may also be available when building for Windows. `sys/stat.h` is available, and we already use it unguarded in other code. So move the defines into bdb, after the stat.h include, and remove compat from bdb.cpp.
ACKs for top commit:
TheCharlatan:
re-ACK 54e4061189
hebasto:
ACK 54e4061189, I have reviewed the code and it looks OK.
Tree-SHA512: b75bb120654b4dec9ccc83aa407ee1a50969fec92f196a3722ec51282b91ac50e455af04f07211f3e93270ab83660f1efdeef43928b44b1e4296f6b06ea807c8
Include a test that checks whether the first argument of
scantxoutset RPC call is required. The rpc call should fail if
the "start" argument is not provided.
This makes it easier to handle descriptor strings manually. E.g. an RPC call that takes an array of descriptors can now use '["desc": ".../0h/..."]'.
Both markers can still be parsed. The default for new descriptors is changed to h. In normalized form h is also used. For private keys the chosen marker is preserved in a round trip.
The hdkeypath field in getaddressinfo is also impacted by this change.
f842ed9a40 test: refactor: replace unnecessary `BytesIO` uses (Sebastian Falbesoner)
Pull request description:
Rather than needing to create intermediate stream variables, we can use helper functions like `tx_from_hex` instead or access the result directly, leading both to increased readability and less code.
ACKs for top commit:
stickies-v:
ACK f842ed9a40
brunoerg:
crACK f842ed9a40
aureleoules:
ACK f842ed9a40 - It seems that these are the only instances that can be changed and it simplifies test code.
Tree-SHA512: 7f4fd7a26720d1988bf27f66c817ff6cd7060350a3be62d28e7848c768fd43578719ca475193d4057ccf4f2458af18564fd513fe3a1d458a11c799927c12bd65
Add an option that when passed, will disable hardening options, and
pass `--disable-hardening` through to configure. Due to the way
we link libssp for Windows builds, they now fail (after #27118),
if building with depends, and configuring with --disable-hardening.
See:
https://github.com/bitcoin/bitcoin/pull/27118#issuecomment-1492606272.
This change would add a depends opiton such that, if someone wants to
build with, for windows, without hardening, they can do so. This may
also be useful when building for debugging.
The following cases were covered:
Case 1: No coin control selected coins.
- 'useAvailableBalance' should fill the amount edit box with the total available balance.
Case 2: With coin control selected coins.
- 'useAvailableBalance' should fill the amount edit box with the sum of the selected coins values.
The previous behavior for getAvailableBalance when coin control
has selected coins was to return the sum of them. Instead, we
are currently returning the wallet's available total balance minus
the selected coins total amount.
This turns into a GUI-only issue for the "use available balance"
button when the user manually select coins in the send screen.
Reason:
We missed to update the GetAvailableBalance function to include
the coin control selected coins on #25685.
Context:
Since #25685 we skip the selected coins inside `AvailableCoins`,
the reason is that there is no need to traverse the wallet's
txes map just to get coins that can directly be fetched by
their id.
