Bitcoin Core Github
44 subscribers
120K links
Download Telegram
👍 hebasto approved a pull request: "ci: Remove deprecated container.greedy"
(https://github.com/bitcoin/bitcoin/pull/28024#pullrequestreview-1513148988)
ACK fac14c4e498f2d38b8b337d4c145129d07403a6d.

Mentioning "timeouts" in the commit message looks unrelated.
🤔 ajtowns reviewed a pull request: "Fix potential network stalling bug"
(https://github.com/bitcoin/bitcoin/pull/27981#pullrequestreview-1513120096)
> The core issue is that whenever our optimistic send fails to fully send a message, we do subsequently not even select() for receiving; if it then turns out that sending is not possible either, no progress is made at all. To address this, the solution used in this PR is to still select() for both sending and receiving when an optimistic send fails, but skip receiving if sending succeeded, and (still) doesn't fully drain the send queue.

AIUI (correct me if I'm wrong!) the backpressure we do i
...
💬 ajtowns commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#discussion_r1252173948)
Would it make sense to introduce a method `bool CNode::WantsToSend() const { return !pnode->vSendMsg.empty(); }` method, and use that here and instead of returning a `pair<X, bool>` above?
🤔 fjahr reviewed a pull request: "Rework validation logic for assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/27746#pullrequestreview-1512960883)
Code review ACK 03dfe04075668aa3b99a5d6d68adfcecffca0593

(modulo my question about the rev file management)

I will also sync a node with this code and report if I see anything unusual in the coming days.
💬 fjahr commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1252074194)
I must either be missing something about this line or it might be unnecessary. If I understand correctly this is updating `m_undo_height_in_last_blockfile` when the next block is connected after the flush happened. However, why not update it right away when the flush happens? I tried that by removing this line it seems like all tests are still passing without it.
💬 dooglus commented on issue "bitcoind hangs waiting for `g_requests.empty()`":
(https://github.com/bitcoin/bitcoin/issues/27722#issuecomment-1620521045)
> Do you happen to know what libevent version you were on?

It looks like version 2.1.12.

```
$ dpkg -l | grep libevent
ii libev4:amd64 1:4.33-1 amd64 high-performance event loop library modelled after libevent
ii libevent-2.1-7:amd64 2.1.12-stable-1 amd64 Asynchronous event notification library
ii libevent-core-2.1-7:amd64
...
💬 luke-jr commented on pull request "wallet: bugfix, always use apostrophe for spkm descriptor ID":
(https://github.com/bitcoin/bitcoin/pull/27920#discussion_r1252222085)
Why not `UNKNOWN_DESCRIPTOR` instead?
🚀 fanquake merged a pull request: "ci: Remove deprecated container.greedy"
(https://github.com/bitcoin/bitcoin/pull/28024)
💬 luke-jr commented on pull request "wallet: bugfix, always use apostrophe for spkm descriptor ID":
(https://github.com/bitcoin/bitcoin/pull/27920#issuecomment-1620528011)
Tag for backport?
💬 fanquake commented on pull request "wallet: bugfix, always use apostrophe for spkm descriptor ID":
(https://github.com/bitcoin/bitcoin/pull/27920#issuecomment-1620530627)
Not if this has (as I understand it) only been broken on master?
💬 achow101 commented on pull request "wallet: bugfix, always use apostrophe for spkm descriptor ID":
(https://github.com/bitcoin/bitcoin/pull/27920#discussion_r1252228384)
The descriptor isn't unknown. It's a corruption error as the id written in the database doesn't match the id that we calculate.
💬 achow101 commented on pull request "wallet: bugfix, always use apostrophe for spkm descriptor ID":
(https://github.com/bitcoin/bitcoin/pull/27920#issuecomment-1620534607)
> Tag for backport?

No need, only affects master.
📝 pablomartin4btc converted_to_draft a pull request: "httpserver, rest: improving URI validation"
(https://github.com/bitcoin/bitcoin/pull/27253)
This PR contains an isolated enhancement of the segfault bugfix https://github.com/bitcoin/bitcoin/pull/27468 (already merged), improving the way we handle the URI validation, which will be performed only once instead of on each query parameter of a rest service endpoint.
💬 pablomartin4btc commented on pull request "httpserver, rest: improving URI validation":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1620535396)
After a chat with @stickies-v where we were discussing different approaches on this enhancement and other details regarding fuzz testing, libevent functionality and each commit intention, I've decided to put this onto draft. Firstly we would need to define the purpose of the `httprequest` object, do we want/ need the `httprequest` obj to exist even with an invalid request?, as @stickies-v [raised his concerns before](https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1481163868), it seem
...
💬 ajtowns commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1620538136)
> I'm starting to think that something closer to your idea here is right: trying ancestor sets of every transaction in the linearization in order, if the ancestor set feerate is suffiicently high. This indeed won't deal with multiple-children-pay-for-parent cases perfectly, but including everything connected may be too much as well. I'll try to think about this more.

I don't think handling multiple-children-pay-for-parent cases perfectly should be a goal here -- we're not fixing eviction (or
...
💬 luke-jr commented on pull request "exclude ipc scheme from port check":
(https://github.com/bitcoin/bitcoin/pull/28020#issuecomment-1620538924)
nit: Rebasing onto bbbf89a9de0757c44880495244f90967f7147c0d would enable a clean merge to 25.x also
💬 MarcoFalke commented on pull request "descriptors: Add a KEY expression representing a list of individual keys":
(https://github.com/bitcoin/bitcoin/pull/26626#issuecomment-1620571802)
Needs rebase if still relevant
💬 MarcoFalke commented on pull request "wallet: don't duplicate change output if already exist":
(https://github.com/bitcoin/bitcoin/pull/27601#issuecomment-1620572868)
From CI https://cirrus-ci.com/task/4675435070488576?logs=ci#L3365:

```
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/wallet_fundrawtransaction.py", line 337, in test_double_change
assert_equal(wallet.gettransaction(desc_tx['vin'][0]['txid'])['amount'], Decimal(50)) # assert input value
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i
...
💬 MarcoFalke commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1620574057)
> Looks like the fuzz target doesn't compile on windows?

Looks like this still wasn't addressed?
📝 luke-jr opened a pull request: "Fix issues in ZMQ error handling"
(https://github.com/bitcoin/bitcoin/pull/28029)
Behaves better if abnormal issues occur