🚀 achow101 merged a pull request: "refactor: Reduce memory copying operations in bech32 encoding"
(https://github.com/bitcoin/bitcoin/pull/29607)
(https://github.com/bitcoin/bitcoin/pull/29607)
💬 achow101 commented on pull request "netbase: extend CreateSock() to support creating arbitrary sockets":
(https://github.com/bitcoin/bitcoin/pull/30202#issuecomment-2166154443)
Silent merge conflict
```
../../../src/test/fuzz/i2p.cpp:30:16: error: no viable overloaded '='
30 | CreateSock = [&fuzzed_data_provider](const sa_family_t&) {
| ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
31 | return std::make_unique<FuzzedSock>(fuzzed_data_provider);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
32 | };
| ~
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/14.1.1/../../../../include/c++
...
(https://github.com/bitcoin/bitcoin/pull/30202#issuecomment-2166154443)
Silent merge conflict
```
../../../src/test/fuzz/i2p.cpp:30:16: error: no viable overloaded '='
30 | CreateSock = [&fuzzed_data_provider](const sa_family_t&) {
| ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
31 | return std::make_unique<FuzzedSock>(fuzzed_data_provider);
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
32 | };
| ~
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/14.1.1/../../../../include/c++
...
💬 paplorinc commented on pull request "refactor: Reduce memory copying operations in bech32 encoding":
(https://github.com/bitcoin/bitcoin/pull/29607#issuecomment-2166248133)
Thanks for the reviews @josibake, @sipa, @achow101.
There's a similar small optimization for [transaction input/output allocations](https://github.com/bitcoin/bitcoin/pull/30093) as well, I'd appreciate a review.
(https://github.com/bitcoin/bitcoin/pull/29607#issuecomment-2166248133)
Thanks for the reviews @josibake, @sipa, @achow101.
There's a similar small optimization for [transaction input/output allocations](https://github.com/bitcoin/bitcoin/pull/30093) as well, I'd appreciate a review.
💬 davidgumberg commented on pull request "Lint: improve subtree exclusion":
(https://github.com/bitcoin/bitcoin/pull/29965#issuecomment-2166387546)
Rebased over master since #30219 was merged.
(https://github.com/bitcoin/bitcoin/pull/29965#issuecomment-2166387546)
Rebased over master since #30219 was merged.
💬 mzumsande commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1638706909)
Some checks are within `ActivateSnapshot()`, others, like the added one, are in the rpc code (which means that they can't be covered in unit tests for that function). Do you know if there is a reason for that?
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1638706909)
Some checks are within `ActivateSnapshot()`, others, like the added one, are in the rpc code (which means that they can't be covered in unit tests for that function). Do you know if there is a reason for that?
💬 ryanofsky commented on pull request "Multiprocess bitcoin":
(https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-2166556815)
re: https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-2158932014
> It is possible there is a bug here, though so if you want to post an issue to the libmultiprocess github project so I can look into it more, that would be helpful.
To follow up on this, I actually found two separate bugs which were causing wallet objects not to be destroyed when the node process is killed. Coincidentally a new test was added recently in #26606 which reproduces this problem, because it actually test
...
(https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-2166556815)
re: https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-2158932014
> It is possible there is a bug here, though so if you want to post an issue to the libmultiprocess github project so I can look into it more, that would be helpful.
To follow up on this, I actually found two separate bugs which were causing wallet objects not to be destroyed when the node process is killed. Coincidentally a new test was added recently in #26606 which reproduces this problem, because it actually test
...
👍 ryanofsky approved a pull request: "Introduce Mining interface"
(https://github.com/bitcoin/bitcoin/pull/30200#pullrequestreview-2116692613)
Code review ACK 5666af2c6e4924bf8e358ab5121c88d14cb085df, just rebased since last review
(https://github.com/bitcoin/bitcoin/pull/30200#pullrequestreview-2116692613)
Code review ACK 5666af2c6e4924bf8e358ab5121c88d14cb085df, just rebased since last review
⚠️ 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`