💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-1880891111)
Sorry for the slow response. I was away buit am now back.

Thanks for the review, I agree that setting special bits is uninteresting for cookie files (I was really trying to keep the subfunctions generic in case we wanted to re-use them elsewhere). I've now removed ability to set special bits.

I think a little on switching to string-based settings, i.e. `-rpccookieperms=group`, but this quickly gets more ugly inside, as IMO the usual use-case is to be setting multiple values in the same per
...

🤔 glozow reviewed a pull request: "Cluster size 2 package rbf"
(https://github.com/bitcoin/bitcoin/pull/28984#pullrequestreview-1808799983)
Combed through the code a bit more. Generally approach ACK for "cluster 2 replace cluster 2" idea.

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1444482970)
Looks correct. If you're willing to entertain a suggestion to make it more readable, I think the following logic is easier to follow and the strings less ambiguous. Since "desc count' and "anc count" usually mean inclusive of tx, it's confusing if it's different in this function.

```suggestion
// Ancestor and descendant counts are inclusive of the tx itself.
const auto ancestor_count{direct_conflict->GetCountWithAncestors()};
const auto descendant_count{direct_confl
...

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1444457214)
Maybe add "Each entry must be part of a cluster with size <=2."

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1444555154)
Just realized this log is pretty nonsensical for package RBF, and also whenever there are multiple transactions being replaced in by a single one.

Maybe this log should be:
```
LogPrint(BCLog::MEMPOOL, "replacing mempool tx %s (wtxid=%s, fees=%s, vsize=%s). New tx %s (wtxid=%s, fees=%s, vsize=%s)\n",
it->GetTx().GetHash().ToString(),
it->GetTx().GetWitnessHash().ToString(),
it->GetFee(),
it->GetTxSize(),

...

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1444505904)
56e2fb281a0e62cb70e610370c2dce9e79bc05f0

We copy this list into every transaction's `MempoolAcceptResult` (which does makes sense since both the parent and the child participated in the replacement of these txns - that's probably helpful for showing package RBFs in the RPC results). But the duplicates are a problem when we add to `vExtraTxnForCompact` in p2p:
https://github.com/bitcoin/bitcoin/blob/04b9df0f9fd95e907b2e8bf823d63e9dfb37b4ad/src/net_processing.cpp#L4254-L4256

Possible soluti
...

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1444492292)
nit: good place to use annotations to double-check we're passing in the right fees:
```suggestion
if (const auto err_string{PaysForRBF(/*original_fees=*/m_conflicting_fees, /*replacement_fees=*/m_total_modified_fees, m_total_vsize,
m_pool.m_incremental_relay_feerate, child_hash)}) {
```

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1444495751)
Maybe add a test case for `CheckConflictTopology` where our 1 parent has 2 children, or our 1 child has 2 parents

💬 kristapsk commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-1881022140)
> I think a little on switching to string-based settings, i.e. `-rpccookieperms=group`, but this quickly gets more ugly inside, as IMO the usual use-case is to be setting multiple values in the same permission. i.e. setting just `user` (`040`) means that the owner-user will NOT be able to read the cookie, as user is checked against owner before group, and if a match is found, with no read perms, the checks are exited. I feel that users are likely to want to set `group` in _addition_ to `user`, t
...

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1444669440)
dealing with duplicates seems strictly superior, will take a look, thanks!

💬 willcl-ark commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-1881049934)
> > I think a little on switching to string-based settings, i.e. `-rpccookieperms=group`, but this quickly gets more ugly inside, as IMO the usual use-case is to be setting multiple values in the same permission. i.e. setting just `user` (`040`) means that the owner-user will NOT be able to read the cookie, as user is checked against owner before group, and if a match is found, with no read perms, the checks are exited. I feel that users are likely to want to set `group` in _addition_ to `user`,
...

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1444673561)
the more work solutions would be to refactor everything to have a "subpackage eval" state, but juice is probably not worth the squeeze until cluster mempool, where "chunk eval" is likely the natural boundary

💬 sipa commented on pull request "Weaken serfloat tests":
(https://github.com/bitcoin/bitcoin/pull/29192#issuecomment-1881052463)
@fanquake What if you drop the last commit here?

💬 kristapsk commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-1881053840)
> I don't think enabling write perms on a cookie (ever?!) that bitcoind has created makes sense?

Ahh, yes, `400`, `440`, `444`.

💬 hebasto commented on issue "qa: No access to `debug.log` in functional tests on Windows":
(https://github.com/bitcoin/bitcoin/issues/28529#issuecomment-1881119075)
https://github.com/bitcoin/bitcoin/actions/runs/7433919780/job/20227517742:
```
2024-01-06T21:17:20.926000Z TestFramework (INFO): Tests successful



stderr:
Traceback (most recent call last):

File "D:\a\bitcoin\bitcoin/test/functional/tool_utxo_to_sqlite.py", line 111, in <module>

UtxoToSqliteTest().main()

File "D:\a\bitcoin\bitcoin\test\functional\test_framework\test_framework.py", line 154, in main

exit_code = self.shutdown()

^^^^^^^^^^^^^^^

...

💬 apoelstra commented on pull request "RFC: Deprecate libconsensus":
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1881121970)
> Would something along the lines of https://github.com/ajtowns/bitcoin/commits/202309-evalscript/ be an interesting alternative?

For my part I would rather have a library function than a CLI tool.

💬 recursive-rat4 commented on pull request "net: create I2P sessions using both ECIES-X25519 and ElGamal encryption":
(https://github.com/bitcoin/bitcoin/pull/29200#issuecomment-1881124957)
ACK 9d728916b27e18efc6f8839770ed5ec14789fc08

I2P 2.3.0 shows both encryption keys.

🤔 furszy reviewed a pull request: "sqlite: Disallow writing from multiple `SQLiteBatch`s"
(https://github.com/bitcoin/bitcoin/pull/29112#pullrequestreview-1809248451)
Another possible deadlock can arise when the txn abortion fails during the destruction of the batch object.
Essentially, if the txn abortion fails and the object is destructed, the semaphore remains locked indefinitely, leading to a deadlock in any subsequent write operation.

Made a test replicating this behavior; cherry-pick 5d2b79a65dcb10abdd349471c125595d677909ab and fbd59f89d3f5ed7bc026efa47d1f78c5cee04291 (from https://github.com/furszy/bitcoin-core/commits/sqlite-concurrent-writes/ )

💬 fanquake commented on pull request "Weaken serfloat tests":
(https://github.com/bitcoin/bitcoin/pull/29192#issuecomment-1881144857)
> What if you drop the last commit here?

Tests are passing: https://github.com/Homebrew/homebrew-core/actions/runs/7448770957/job/20264045705?pr=156745.

🚀 fanquake merged a pull request: "wallet: Migrate entire address book entries to watchonly and solvables too"
(https://github.com/bitcoin/bitcoin/pull/28610)