💬 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(),
...
(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
...
(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)}) {
```
(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
(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
...
(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!
(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`,
...
(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
(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?
(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`.
(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()
^^^^^^^^^^^^^^^
...
(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.
(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.
(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/ )
(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.
(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)
(https://github.com/bitcoin/bitcoin/pull/28610)
📝 hebasto opened a pull request: "build: Drop `ALLOW_HOST_PACKAGES` support in depends"
(https://github.com/bitcoin/bitcoin/pull/29203)
The `ALLOW_HOST_PACKAGES` variable was introduced in bitcoin#10508 "to speed up build and avoid timeout".
It is no longer the case for our CI infrastructure, which uses self- hosted persistent workers and depends caching.
In the current circumstances, it does not seem worth porting this feature to the upcoming CMake-based build system.
(https://github.com/bitcoin/bitcoin/pull/29203)
The `ALLOW_HOST_PACKAGES` variable was introduced in bitcoin#10508 "to speed up build and avoid timeout".
It is no longer the case for our CI infrastructure, which uses self- hosted persistent workers and depends caching.
In the current circumstances, it does not seem worth porting this feature to the upcoming CMake-based build system.
📝 furszy opened a pull request: "test: wallet migration, add coverage for tx extra data"
(https://github.com/bitcoin/bitcoin/pull/29204)
Quick follow-up to #28610, coming from https://github.com/bitcoin/bitcoin/pull/28610#pullrequestreview-1802823938.
Verifying that the 'replaced_by_txid' and 'replaces_txid' tx data is preserved after migration,
as well as the extra tx comments.
(https://github.com/bitcoin/bitcoin/pull/29204)
Quick follow-up to #28610, coming from https://github.com/bitcoin/bitcoin/pull/28610#pullrequestreview-1802823938.
Verifying that the 'replaced_by_txid' and 'replaces_txid' tx data is preserved after migration,
as well as the extra tx comments.
💬 hebasto commented on issue "CMake-based build system tracking issue":
(https://github.com/bitcoin/bitcoin/issues/28607#issuecomment-1881191524)
Heads up: here are suggestions regarding the current features that are _not_ expected being ported to the CMake-based build system:
- https://github.com/bitcoin/bitcoin/pull/29185
- https://github.com/bitcoin/bitcoin/pull/29189
- https://github.com/bitcoin/bitcoin/pull/29203
(https://github.com/bitcoin/bitcoin/issues/28607#issuecomment-1881191524)
Heads up: here are suggestions regarding the current features that are _not_ expected being ported to the CMake-based build system:
- https://github.com/bitcoin/bitcoin/pull/29185
- https://github.com/bitcoin/bitcoin/pull/29189
- https://github.com/bitcoin/bitcoin/pull/29203
💬 instagibbs commented on pull request "test: Add test case for spending bare multisig":
(https://github.com/bitcoin/bitcoin/pull/29120#discussion_r1444815132)
> If the tx-to-be-spent is already confirmed, there shouldn't be any problem having a spend of a 20-of-20 checkmultisig in the mempool
we have tests for this? If not, this is probably the right thing to cover I guewss :)
(https://github.com/bitcoin/bitcoin/pull/29120#discussion_r1444815132)
> If the tx-to-be-spent is already confirmed, there shouldn't be any problem having a spend of a 20-of-20 checkmultisig in the mempool
we have tests for this? If not, this is probably the right thing to cover I guewss :)