⚠️ ismaelsadeeq opened an issue: "Mini miner `AncestorFeerateComparator` Signed Integer Overflow"
(https://github.com/bitcoin/bitcoin/issues/30284)
While working on #30079, I noticed that using multiplication to compare fee rates results in a signed integer overflow.
https://github.com/bitcoin/bitcoin/blob/fcc3b653dc2bd5683193a836556de07bea4d1b11/src/node/mini_miner.cpp#L191-L195
The policy estimator fuzz tests generate transaction fee and sigops values using `ConsumeTxMemPoolEntry`.
https://github.com/bitcoin/bitcoin/blob/fcc3b653dc2bd5683193a836556de07bea4d1b11/src/test/fuzz/util/mempool.cpp#L17-L31
Here https://github.com/bitcoi
...
(https://github.com/bitcoin/bitcoin/issues/30284)
While working on #30079, I noticed that using multiplication to compare fee rates results in a signed integer overflow.
https://github.com/bitcoin/bitcoin/blob/fcc3b653dc2bd5683193a836556de07bea4d1b11/src/node/mini_miner.cpp#L191-L195
The policy estimator fuzz tests generate transaction fee and sigops values using `ConsumeTxMemPoolEntry`.
https://github.com/bitcoin/bitcoin/blob/fcc3b653dc2bd5683193a836556de07bea4d1b11/src/test/fuzz/util/mempool.cpp#L17-L31
Here https://github.com/bitcoi
...
💬 ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-2166643540)
> I didn't look too closely into the CI failure reason, but seems like a relatively simple fix, i.e. fixing an signed int overflow:
@josibake overflow is from mini miner `AncestorFeerateComparator`, added a fixup commit here and opened an issue for it.
https://github.com/bitcoin/bitcoin/issues/30284
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-2166643540)
> I didn't look too closely into the CI failure reason, but seems like a relatively simple fix, i.e. fixing an signed int overflow:
@josibake overflow is from mini miner `AncestorFeerateComparator`, added a fixup commit here and opened an issue for it.
https://github.com/bitcoin/bitcoin/issues/30284
🤔 ismaelsadeeq reviewed a pull request: "Introduce Mining interface"
(https://github.com/bitcoin/bitcoin/pull/30200#pullrequestreview-2116792265)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30200#pullrequestreview-2116792265)
Concept ACK
💬 ismaelsadeeq commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#issuecomment-2166662408)
> Would be nice to have this work for create_self_transfer_multi as well
I will pick this up and the outstanding review comments.
(https://github.com/bitcoin/bitcoin/pull/30162#issuecomment-2166662408)
> Would be nice to have this work for create_self_transfer_multi as well
I will pick this up and the outstanding review comments.
👋 sr-gi's pull request is ready for review: "p2p: Fill reconciliation sets (Erlay) attempt 2"
(https://github.com/bitcoin/bitcoin/pull/30116)
(https://github.com/bitcoin/bitcoin/pull/30116)
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#issuecomment-2166667309)
This should be ready for review now.
(https://github.com/bitcoin/bitcoin/pull/30116#issuecomment-2166667309)
This should be ready for review now.
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1638809793)
This is pending. Happy get get feedback on how many to pick (either hardcoded or based on what)
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1638809793)
This is pending. Happy get get feedback on how many to pick (either hardcoded or based on what)
👋 ismaelsadeeq's pull request is ready for review: "Fee Estimation: Ignore all transactions that are CPFP'd"
(https://github.com/bitcoin/bitcoin/pull/30079)
(https://github.com/bitcoin/bitcoin/pull/30079)
👍 benalleng approved a pull request: "doc: archive release notes for v27.1"
(https://github.com/bitcoin/bitcoin/pull/30276#pullrequestreview-2117152664)
ACK for e71a21b
no output for
```
git diff e71a21b641541f4751ac1ea7fd7d33f20a629636:doc/release-notes/release-notes-27.1.md 27.x:doc/release-notes.md
```
shout-out to this nice comment https://github.com/bitcoin/bitcoin/pull/29886#issuecomment-2058543218 for a good way to review these
(https://github.com/bitcoin/bitcoin/pull/30276#pullrequestreview-2117152664)
ACK for e71a21b
no output for
```
git diff e71a21b641541f4751ac1ea7fd7d33f20a629636:doc/release-notes/release-notes-27.1.md 27.x:doc/release-notes.md
```
shout-out to this nice comment https://github.com/bitcoin/bitcoin/pull/29886#issuecomment-2058543218 for a good way to review these
💬 kevkevinpal commented on pull request "test: cover more errors for `signrawtransactionwithkey` RPC":
(https://github.com/bitcoin/bitcoin/pull/30278#issuecomment-2167001364)
ACK [e2779ce](https://github.com/bitcoin/bitcoin/pull/30278/commits/e2779ce98b39e14cada08a654928e798436f5a46)
(https://github.com/bitcoin/bitcoin/pull/30278#issuecomment-2167001364)
ACK [e2779ce](https://github.com/bitcoin/bitcoin/pull/30278/commits/e2779ce98b39e14cada08a654928e798436f5a46)
💬 kevkevinpal commented on pull request "test: Added test coverage to listsinceblock rpc":
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1639084308)
I was getting this error so I opted to use the full path
error message: `OSError: [Errno 18] Invalid cross-device link: '/tmp/bitcoin_func_test_lt7jywkj/node0/regtest/blocks/blk00000.dat.moved' -> 'blk00000.dat'`
but I like the assertions thanks for the suggestion!
updated in [9bf646c](https://github.com/bitcoin/bitcoin/pull/30195/commits/9bf646c4190e55a13bf3a89d1705763c7a833a06)
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1639084308)
I was getting this error so I opted to use the full path
error message: `OSError: [Errno 18] Invalid cross-device link: '/tmp/bitcoin_func_test_lt7jywkj/node0/regtest/blocks/blk00000.dat.moved' -> 'blk00000.dat'`
but I like the assertions thanks for the suggestion!
updated in [9bf646c](https://github.com/bitcoin/bitcoin/pull/30195/commits/9bf646c4190e55a13bf3a89d1705763c7a833a06)
💬 kevkevinpal commented on pull request "log: use error level for critical log messages":
(https://github.com/bitcoin/bitcoin/pull/30255#issuecomment-2167024473)
ACK [fae3a1f](https://github.com/bitcoin/bitcoin/pull/30255/commits/fae3a1f0065064d80ab4c0375a9eaeb666c5dd55)
agree with https://github.com/bitcoin/bitcoin/pull/30255#discussion_r1633007398 but that should be good for a follow up
(https://github.com/bitcoin/bitcoin/pull/30255#issuecomment-2167024473)
ACK [fae3a1f](https://github.com/bitcoin/bitcoin/pull/30255/commits/fae3a1f0065064d80ab4c0375a9eaeb666c5dd55)
agree with https://github.com/bitcoin/bitcoin/pull/30255#discussion_r1633007398 but that should be good for a follow up
💬 kevkevinpal commented on pull request "test: add validation for gettxout RPC response":
(https://github.com/bitcoin/bitcoin/pull/30226#discussion_r1639092018)
Should we use `COIN` here instead of `100000000`
found in `bitcoin/test/functional/test_framework/messages.py`
(https://github.com/bitcoin/bitcoin/pull/30226#discussion_r1639092018)
Should we use `COIN` here instead of `100000000`
found in `bitcoin/test/functional/test_framework/messages.py`
💬 kevkevinpal commented on pull request "test: add validation for gettxout RPC response":
(https://github.com/bitcoin/bitcoin/pull/30226#discussion_r1639093666)
This logs seems a bit vague, a more descriptive log would be useful
(https://github.com/bitcoin/bitcoin/pull/30226#discussion_r1639093666)
This logs seems a bit vague, a more descriptive log would be useful
👍 tdb3 approved a pull request: "test: Added test coverage to listsinceblock rpc"
(https://github.com/bitcoin/bitcoin/pull/30195#pullrequestreview-2117257663)
re ACK 9bf646c4190e55a13bf3a89d1705763c7a833a06
(https://github.com/bitcoin/bitcoin/pull/30195#pullrequestreview-2117257663)
re ACK 9bf646c4190e55a13bf3a89d1705763c7a833a06
💬 tdb3 commented on pull request "test: Added test coverage to listsinceblock rpc":
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1639100512)
nit: could also add an assert (or combine into the one above) to check that `blk_dat_move.exists()`.
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1639100512)
nit: could also add an assert (or combine into the one above) to check that `blk_dat_move.exists()`.
💬 BenWestgate commented on issue "Comply with the XDG Base Directory Specification":
(https://github.com/bitcoin/bitcoin/issues/16733#issuecomment-2167051486)
> I don't think anything in the datadir qualifies as "cache", so at most we could split it up between `XDG_CONFIG_HOME` and `XDG_DATA_HOME`.
This is a great place to start because Bitcoin-Qt already uses `$XDG_CONFIG_HOME/{Bitcoin/Bitcoin-Qt.conf,autostart/bitcoin.desktop}`.
> I see `db.log`, `debug.log`, ...
These go in `$XDG_STATE_HOME`.
>The $XDG_STATE_HOME contains state data that should persist between (application) restarts, but that is not important or portable enough to the u
...
(https://github.com/bitcoin/bitcoin/issues/16733#issuecomment-2167051486)
> I don't think anything in the datadir qualifies as "cache", so at most we could split it up between `XDG_CONFIG_HOME` and `XDG_DATA_HOME`.
This is a great place to start because Bitcoin-Qt already uses `$XDG_CONFIG_HOME/{Bitcoin/Bitcoin-Qt.conf,autostart/bitcoin.desktop}`.
> I see `db.log`, `debug.log`, ...
These go in `$XDG_STATE_HOME`.
>The $XDG_STATE_HOME contains state data that should persist between (application) restarts, but that is not important or portable enough to the u
...
📝 sipa opened a pull request: "Low-level cluster linearization code: merging & postprocessing"
(https://github.com/bitcoin/bitcoin/pull/30285)
Depends on #30126. #28676 will build on this PR.
This adds the algorithms for merging & postprocessing linearizations.
The `PostLinearize(depgraph, linearization)` function performs an in-place improvement of `linearization`, using two iterations of the [Linearization post-processing](https://delvingbitcoin.org/t/linearization-post-processing-o-n-2-fancy-chunking/201/8) algorithm. The first running from back to front, the second from front to back.
The `MergeLinearizations(depgraph, lin
...
(https://github.com/bitcoin/bitcoin/pull/30285)
Depends on #30126. #28676 will build on this PR.
This adds the algorithms for merging & postprocessing linearizations.
The `PostLinearize(depgraph, linearization)` function performs an in-place improvement of `linearization`, using two iterations of the [Linearization post-processing](https://delvingbitcoin.org/t/linearization-post-processing-o-n-2-fancy-chunking/201/8) algorithm. The first running from back to front, the second from front to back.
The `MergeLinearizations(depgraph, lin
...
📝 sipa opened a pull request: "Low-level cluster linearization code: optimizations"
(https://github.com/bitcoin/bitcoin/pull/30286)
Depends on #30126.
This improves the candidate search algorithm introduced in the previous PR with a variety of optimizations.
The resulting search algorithm largely follows Section 2 of [How to linearize your cluster](https://delvingbitcoin.org/t/how-to-linearize-your-cluster/303#h-2-finding-high-feerate-subsets-5), though with a few changes:
* Connected component analysis is performed inside the search algorithm (creating initial work items per component for each candidate), rather than
...
(https://github.com/bitcoin/bitcoin/pull/30286)
Depends on #30126.
This improves the candidate search algorithm introduced in the previous PR with a variety of optimizations.
The resulting search algorithm largely follows Section 2 of [How to linearize your cluster](https://delvingbitcoin.org/t/how-to-linearize-your-cluster/303#h-2-finding-high-feerate-subsets-5), though with a few changes:
* Connected component analysis is performed inside the search algorithm (creating initial work items per component for each candidate), rather than
...
💬 maflcko commented on pull request "test: Added test coverage to listsinceblock rpc":
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1639367894)
> (Could even `blk_dat_moved.rename(blk_dat.name)` to avoid repeating the literal `"blk00000.dat"` but meh)
No, this is wrong. https://docs.python.org/3/library/pathlib.html#pathlib.Path.rename says "The target path may be absolute or relative. Relative paths are interpreted relative to the current working directory, not the directory of the Path object."
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1639367894)
> (Could even `blk_dat_moved.rename(blk_dat.name)` to avoid repeating the literal `"blk00000.dat"` but meh)
No, this is wrong. https://docs.python.org/3/library/pathlib.html#pathlib.Path.rename says "The target path may be absolute or relative. Relative paths are interpreted relative to the current working directory, not the directory of the Path object."