💬 TheCharlatan commented on pull request "depends: Allow PATH with spaces in directory names.":
(https://github.com/bitcoin/bitcoin/pull/28733#discussion_r1377334673)
Is this actually fixing anything though?

💬 vasild commented on pull request "ci: run test_bitcoin with DEBUG_LOG_OUT in RUN_UNIT_TESTS_SEQUENTIAL":
(https://github.com/bitcoin/bitcoin/pull/28736#issuecomment-1786898597)
I deliberately broke some tests to see how this will report the failures. Looks good:

<details>
<summary>CI log from artificial failures</summary>

```
+ LOG=/ci_container_base/ci/scratch/build/test_bitcoin.log
+ DIR_UNIT_TEST_DATA=/ci_container_base/ci/scratch/qa-assets/unit_test_data/
+ LD_LIBRARY_PATH=/ci_container_base/depends/x86_64-pc-linux-gnu/lib
+ /ci_container_base/ci/scratch/out/bin/test_bitcoin --catch_system_errors=no --color_output=false --log_level=test_suite -- DEBUG_LO
...

🤔 hebasto reviewed a pull request: "guix: Zip needs to include all files and set time to SOURCE_DATE_EPOCH"
(https://github.com/bitcoin/bitcoin/pull/28757#pullrequestreview-1705942744)
I've got the same hashes as in https://github.com/bitcoin/bitcoin/pull/28757#issuecomment-1785918138 and https://github.com/bitcoin/bitcoin/pull/28757#issuecomment-1785996627.

The `dist` top directory is redundant and should be avoided:

Also see: https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1610057916

💬 toolsopen commented on issue "sendrawtransaction takes too long":
(https://github.com/bitcoin/bitcoin/issues/28745#issuecomment-1786929653)
One thing I need to state is that all transactions in my wallet are sent and received from the same address, and the read-only wallet mode used is only for broadcast transactions and utxo queries.

💬 toolsopen commented on issue "sendrawtransaction takes too long":
(https://github.com/bitcoin/bitcoin/issues/28745#issuecomment-1786932219)
> sqlite

I tried to use sqlite, but it was prompted that read-only import addresses are not supported.

💬 hebasto commented on pull request "build: remove duplicate `-lminiupnpc` linking":
(https://github.com/bitcoin/bitcoin/pull/28755#discussion_r1377382561)
Running `AC_CHECK_LIB` unconditionally does not make sense. Maybe gate it with `if test "$have_miniupnpc" != "no"`, where `have_miniupnpc` is set to `no` in "not-found" branch of the `AC_CHECK_HEADERS` macro?

💬 fanquake commented on pull request "build: remove duplicate `-lminiupnpc` linking":
(https://github.com/bitcoin/bitcoin/pull/28755#discussion_r1377390119)
I will just change this, along with the natpmp logic at the same time.

💬 hebasto commented on pull request "build: remove duplicate `-lminiupnpc` linking":
(https://github.com/bitcoin/bitcoin/pull/28755#discussion_r1377415844)
The `AC_CHECK_LIB` macro still have to set `have_natpmp=no` if the library not found, as it is used later: https://github.com/bitcoin/bitcoin/blob/9a823890be9a6b4edfe6f32890f61aba9605dbbc/configure.ac#L1750

Same for `have_miniupnpc`: https://github.com/bitcoin/bitcoin/blob/9a823890be9a6b4edfe6f32890f61aba9605dbbc/configure.ac#L1729

💬 fanquake commented on pull request "build: remove duplicate `-lminiupnpc` linking":
(https://github.com/bitcoin/bitcoin/pull/28755#discussion_r1377421995)
I think this is covering an edge case that is very unlikely to happen, but I'll add it in any case.

🚀 fanquake merged a pull request: "refactor: Remove WithParams serialization helper, use SER_PARAMS_OPFUNC"
(https://github.com/bitcoin/bitcoin/pull/28503)

💬 TheCharlatan commented on pull request "build: Introduce internal kernel library":
(https://github.com/bitcoin/bitcoin/pull/28690#issuecomment-1787028895)
Updated 815eaef0e1a30b746aef2a1112bb9b7895d119de -> 4052c2d5b9f84289752093981f475ea2ca5b64e7 ([kernelInternalLib_2](https://github.com/TheCharlatan/bitcoin/tree/kernelInternalLib_2) -> [kernelInternalLib_3](https://github.com/TheCharlatan/bitcoin/tree/kernelInternalLib_3), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelInternalLib_2..kernelInternalLib_3))
* Fixed silent merge conflict.

👍 hebasto approved a pull request: "build: remove duplicate `-lminiupnpc` linking"
(https://github.com/bitcoin/bitcoin/pull/28755#pullrequestreview-1706070279)
ACK b74e449ffa9965f18037f0322ea178e2fe1dbc18

`./configure` output:
- in the master branch:
```
...
checking for miniupnpc/miniupnpc.h... yes
checking for upnpDiscover in -lminiupnpc... yes
checking for miniupnpc/upnpcommands.h... yes
checking for upnpDiscover in -lminiupnpc... (cached) yes
checking for miniupnpc/upnperrors.h... yes
checking for upnpDiscover in -lminiupnpc... (cached) yes
checking whether miniUPnPc API version is supported... yes
checking for natpmp.h... yes
checki
...

📝 fanquake opened a pull request: "guix: update signapple to latest master"
(https://github.com/bitcoin/bitcoin/pull/28759)
Fixes #28449, and removes the need to boostrap Rust, by avoiding the `python-requests` dependency.

Draft for now, as we still need https://github.com/achow101/signapple/pull/11, but anyone should be able to verify the bootstrapping difference with this branch.

Comparing a `--no-substitutes` build of this PR, to master, signapple requires ~1350 _less_ packages to boostrap:
Master derivation - https://gist.github.com/fanquake/dbf69a62c9a78b7ae8c183a160e6d58d
PR derivation - https://gist.gi
...

💬 fanquake commented on pull request "net: improves addnode / m_added_nodes logic":
(https://github.com/bitcoin/bitcoin/pull/28155#discussion_r1377456646)
This should be including `compat.h`, rather than introducing the same networking #ifdef here?

💬 hebasto commented on pull request "guix: update signapple to latest master":
(https://github.com/bitcoin/bitcoin/pull/28759#issuecomment-1787057059)
Concept ACK.

💬 brunoerg commented on pull request "fuzz: call lookup functions before calling `Ban`":
(https://github.com/bitcoin/bitcoin/pull/27935#issuecomment-1787069782)
Rebased and changed the approach in fuzz to call `Ban` only with valid net address/subnet.

💬 fanquake commented on pull request "guix: update time-machine":
(https://github.com/bitcoin/bitcoin/pull/28580#issuecomment-1787095966)
Updated to a newer hash. We've also upstreamed a GCC bump from 10.4.0 to 10.5.0: https://git.savannah.gnu.org/cgit/guix.git/commit/?id=2fbb5398a39bf18e41235891a0740fa0bc4d7a4d.

💬 glozow commented on pull request "refactors for subpackage evaluation":
(https://github.com/bitcoin/bitcoin/pull/28758#discussion_r1377493306)
Ok, renamed this to `IsTopoSortedPackage`, i.e. "is a topologically sorted package" to be more descriptive.

💬 dergoegge commented on pull request "refactors for subpackage evaluation":
(https://github.com/bitcoin/bitcoin/pull/28758#discussion_r1377497946)
can you do the same for `IsConsistent`? e.g. `IsPackageConsistent`

🤔 furszy reviewed a pull request: "gui: fix crash on selecting "Mask values" in transaction view"
(https://github.com/bitcoin-core/gui/pull/774#pullrequestreview-1706180484)
Could also:
```diff
diff --git a/src/qt/transactionview.cpp b/src/qt/transactionview.cpp
@@ -656,7 +656,7 @@
{
// close all dialogs opened from this view
for (QDialog* dlg : m_opened_dialogs) {
- dlg->close();
+ if (dlg) dlg->close();
}
m_opened_dialogs.clear();
}
```