💬 glozow commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1386821034)
same

💬 ismaelsadeeq commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#issuecomment-1802147662)
Force pushed from 09436b21e84cc6cd979fe4942ba1c415c4bc91be to 969f5ec4a8
- Addressed @glozow review comments
- Added a commit that `SyncWithValidationInterfaceQueue` at the start of fee estimation RPC's
- The relationship between `RemovedMempoolTransactionInfo`, `NewMempoolTransactionInfo`, and `TransactionInfo` has been updated to use composition. `TransactionInfo` object is now a member of `RemovedMempoolTransactionInfo` and `NewMempoolTransactionInfo`.

💬 maflcko commented on pull request "ci: remove note re M1 usage":
(https://github.com/bitcoin/bitcoin/pull/28823#issuecomment-1802149189)
lgtm ACK 8cbb6196913b22006dac75f719a2834ab0d6c94f

🤔 glozow reviewed a pull request: "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access"
(https://github.com/bitcoin/bitcoin/pull/28391#pullrequestreview-1720677427)
drahty pong

👍 luke-jr approved a pull request: "rpc: keep `.cookie` file if it was not generated"
(https://github.com/bitcoin/bitcoin/pull/28784#pullrequestreview-1720677514)
utACK, though it might be better to do something like keep the file open and detect if it's the same file/content before we delete it.

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1386826811)
> `SyncState` yeah that would work if you come up with a good enum values.

Maybe `Synced`, `RecentStale`, `Stale`?
Named the second and third "stale" because these states have no in-flight block requests.

Still, this could also be easily changed in the future if we come up with better names.

💬 maflcko commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378898930)
clang-format new code?

```diff
diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h
index 1b188e0f49..d323a320b5 100644
--- a/src/primitives/transaction.h
+++ b/src/primitives/transaction.h
@@ -189,13 +189,12 @@ public:

struct CMutableTransaction;

-struct TransactionSerParams
-{
+struct TransactionSerParams {
const bool allow_witness;
SER_PARAMS_OPFUNC
};
-static constexpr TransactionSerParams TX_WITH_WITNESS{.allow_witness=true};
-static co
...

💬 luke-jr commented on pull request "guix: switch to 6.1 kernel headers over 5.15":
(https://github.com/bitcoin/bitcoin/pull/28786#issuecomment-1802153356)
>Note that using an older version of the kernel headers inside Guix, is not a "hack" for compatibility, and is explicitly recommended against by glibc:

Yet boost doesn't make the same compatibility guarantees, and needs to be checked.

💬 achow101 commented on pull request "init: completely remove `-zapwallettxes` (remaining hidden option)":
(https://github.com/bitcoin/bitcoin/pull/28787#issuecomment-1802168625)
ACK 5039c346ca87d6112ea1eb124bdc622ba9e9a513

🚀 achow101 merged a pull request: "init: completely remove `-zapwallettxes` (remaining hidden option)"
(https://github.com/bitcoin/bitcoin/pull/28787)

👍 kristapsk approved a pull request: "rpc: keep `.cookie` file if it was not generated"
(https://github.com/bitcoin/bitcoin/pull/28784#pullrequestreview-1720723346)
utACK 7cb9367157eb42ee06bc6fa024522cc14a80138d

💬 pinheadmz commented on issue "whiteconnections should be re-added":
(https://github.com/bitcoin/bitcoin/issues/8798#issuecomment-1802198296)
Ping @theymos @asoltys take a look at #27600 please and leave a comment if this is still an issue that concerns you

📝 willcl-ark opened a pull request: "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes"
(https://github.com/bitcoin/bitcoin/pull/28824)
Closes: #27795
Closes: #7996

Previously `ScriptToAsmStr` returned hex-encoded integers, except if data length was <= 4 bytes, in which case it displayed using decimal encoding.

Remove the decimal encoding carve-out for small pushes and always display hex results from `decodescript` RPC.

There were [other suggestions](https://github.com/bitcoin/bitcoin/issues/27795#issuecomment-1692870456) for approaches in #27795, but IMO moving to hex-only-always makes the most sense.

💬 luke-jr commented on pull request "snapshots: don't core dump when running -checkblockindex after `loadtxoutset`":
(https://github.com/bitcoin/bitcoin/pull/28791#issuecomment-1802220054)
Maybe snapshots should include it?

💬 luke-jr commented on pull request "Embedding ASMap files as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-1802223047)
Why is the option of loading it from a file even in dev builds, not considered?

💬 maflcko commented on pull request "snapshots: don't core dump when running -checkblockindex after `loadtxoutset`":
(https://github.com/bitcoin/bitcoin/pull/28791#discussion_r1386888196)
For reference, this whole assert block is likely wrong, see the thread at https://github.com/bitcoin/bitcoin/pull/28562#discussion_r1350470707

💬 maflcko commented on pull request "AssumeUTXO follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28562#discussion_r1386889955)
Another report, which could replicate the crash, see #https://github.com/bitcoin/bitcoin/pull/28791

💬 luke-jr commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#issuecomment-1802243288)
Concept NACK

>The motivation is that we should maintain the tooling for de- and encoding asmap files within the bitcoin core repository because it is not possible to use an asmap file that is not encoded.

That's not a reason. Ideally, the current repo should be split into several itself - but we have technical hurdles to get to that point. No reason to add more here "just because"

💬 pinheadmz commented on pull request "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full":
(https://github.com/bitcoin/bitcoin/pull/27600#issuecomment-1802246176)
@naumenkogs If the user has *also* set a low `maxconnections` value then [`SelectNodeToEvict()`](https://github.com/bitcoin/bitcoin/blob/19d1ba1b4141f744cca291ff2d99d0b8ffcf946d/src/node/eviction.cpp#L186-L197) might return null.

Quick math 4+8+4+8+4 = 28 protected nodes. Plus 8 full outbound and 2 block only = 38. So if a user runs a full node on limited hardware (like my Raspberry Pi at home) and have something like `-maxconnections=30` then even with the existing `whitebind` permissions, t
...

🤔 luke-jr requested changes to a pull request: "rpc: Add script verification flags to getdeploymentinfo"
(https://github.com/bitcoin/bitcoin/pull/28806#pullrequestreview-1720807012)
Getting some deja vu here - is there another PR for this already?