💬 glozow commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#issuecomment-1573521731)
> > redownloading orphans if we cannot afford to protect them
>
> Is the idea here that protection is a bandiwdth optimization to avoid fetching an orphan twice in a row in the presence of malicious peers or random churn? Reasonable if so, just want this to be explicitly stated if so.
Yes exactly. Worse is we want to avoid a situation where we requested the rest of the package while the orphan was in our orphanage, but then it fell out while we were waiting for the rest of the transactions
...
(https://github.com/bitcoin/bitcoin/pull/27742#issuecomment-1573521731)
> > redownloading orphans if we cannot afford to protect them
>
> Is the idea here that protection is a bandiwdth optimization to avoid fetching an orphan twice in a row in the presence of malicious peers or random churn? Reasonable if so, just want this to be explicitly stated if so.
Yes exactly. Worse is we want to avoid a situation where we requested the rest of the package while the orphan was in our orphanage, but then it fell out while we were waiting for the rest of the transactions
...
💬 TheCharlatan commented on pull request "kernel: Remove shutdown from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27711#discussion_r1214254203)
I don't really understand the improvement in the context of achieving step 2 of the kernel library project between the solution you implemented now and what is implemented currently here in the first commit, besides it being a big step towards de-globalizing. One of my goals for this PR here is to stop the kernel relying on code that handles platform-dependent blocking while still being signal-aware (e.g. the bulk of what is currently in shutdown). I don't see why such logic should exist in a st
...
(https://github.com/bitcoin/bitcoin/pull/27711#discussion_r1214254203)
I don't really understand the improvement in the context of achieving step 2 of the kernel library project between the solution you implemented now and what is implemented currently here in the first commit, besides it being a big step towards de-globalizing. One of my goals for this PR here is to stop the kernel relying on code that handles platform-dependent blocking while still being signal-aware (e.g. the bulk of what is currently in shutdown). I don't see why such logic should exist in a st
...
💬 ChrisCho-H commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#issuecomment-1573587610)
@MarcoFalke would u mind to review this?
(https://github.com/bitcoin/bitcoin/pull/27109#issuecomment-1573587610)
@MarcoFalke would u mind to review this?
💬 ryanofsky commented on pull request "walletdb: Add PrefixCursor":
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1214272570)
I think I agree that current `Serialize(Span<char>)` overloads could be a little dangerous:
https://github.com/bitcoin/bitcoin/blob/6a560aceb75e618f3106a8850e053cd8de87616a/src/serialize.h#L202-L205
Because if a variable-length container could be implicitly converted to span of these types, it might incorrectly match one of these overloads and be serialized as a fixed length span without a length prefix, and no longer be deserializable as the original container.
If that is a real proble
...
(https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1214272570)
I think I agree that current `Serialize(Span<char>)` overloads could be a little dangerous:
https://github.com/bitcoin/bitcoin/blob/6a560aceb75e618f3106a8850e053cd8de87616a/src/serialize.h#L202-L205
Because if a variable-length container could be implicitly converted to span of these types, it might incorrectly match one of these overloads and be serialized as a fixed length span without a length prefix, and no longer be deserializable as the original container.
If that is a real proble
...
🤔 Sjors reviewed a pull request: "contrib: verify torrents with verify-binary.py"
(https://github.com/bitcoin/bitcoin/pull/27762#pullrequestreview-1457289222)
Successfully tested on macOS 13.4 against v25.0.
Lightly code reviewed a6dcf2cb0817143beacf0b9004191a0dd6d2e43c.
Do we want to use the .asc file from the torrent, or should we just get it from the Guix signature repo? The latter may have more signatures, as they keep coming in after release.
(https://github.com/bitcoin/bitcoin/pull/27762#pullrequestreview-1457289222)
Successfully tested on macOS 13.4 against v25.0.
Lightly code reviewed a6dcf2cb0817143beacf0b9004191a0dd6d2e43c.
Do we want to use the .asc file from the torrent, or should we just get it from the Guix signature repo? The latter may have more signatures, as they keep coming in after release.
💬 Sjors commented on pull request "contrib: verify torrents with verify-binary.py":
(https://github.com/bitcoin/bitcoin/pull/27762#discussion_r1214312611)
a6dcf2cb0817143beacf0b9004191a0dd6d2e43c Did you mean to change the indentation here? Maybe do so in the first refactor commit?
(https://github.com/bitcoin/bitcoin/pull/27762#discussion_r1214312611)
a6dcf2cb0817143beacf0b9004191a0dd6d2e43c Did you mean to change the indentation here? Maybe do so in the first refactor commit?
💬 Sjors commented on pull request "contrib: verify torrents with verify-binary.py":
(https://github.com/bitcoin/bitcoin/pull/27762#discussion_r1214314041)
a6dcf2cb0817143beacf0b9004191a0dd6d2e43c: did you mean to do this in the refactor commit?
(https://github.com/bitcoin/bitcoin/pull/27762#discussion_r1214314041)
a6dcf2cb0817143beacf0b9004191a0dd6d2e43c: did you mean to do this in the refactor commit?
💬 ryanofsky commented on pull request "kernel: Remove shutdown from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27711#discussion_r1214342650)
> One of my goals for this PR here is to stop the kernel relying on code that handles platform-dependent blocking while still being signal-aware (e.g. the bulk of what is currently in shutdown). I don't see why such logic should exist in a standalone kernel library. Do you have a different perspective here?
Yes, I don't think we should do this because it gets in the way of the kernel having a nice and future-proof API.
I think we both agree that applications using libbitcoinkernel should h
...
(https://github.com/bitcoin/bitcoin/pull/27711#discussion_r1214342650)
> One of my goals for this PR here is to stop the kernel relying on code that handles platform-dependent blocking while still being signal-aware (e.g. the bulk of what is currently in shutdown). I don't see why such logic should exist in a standalone kernel library. Do you have a different perspective here?
Yes, I don't think we should do this because it gets in the way of the kernel having a nice and future-proof API.
I think we both agree that applications using libbitcoinkernel should h
...
📝 willcl-ark opened a pull request: "init: deduplicate added connections"
(https://github.com/bitcoin/bitcoin/pull/27804)
Fixes #5299
Previously connections may be added to both m_added_nodes (-addnode) and m_specified_outgoing (-connect) if duplicated in config.
Deduplicate the lists before starting connection manager.
There are ways to bypass this simple de-duplication as it relies on string comparison, but it gets us most of the way.
(https://github.com/bitcoin/bitcoin/pull/27804)
Fixes #5299
Previously connections may be added to both m_added_nodes (-addnode) and m_specified_outgoing (-connect) if duplicated in config.
Deduplicate the lists before starting connection manager.
There are ways to bypass this simple de-duplication as it relies on string comparison, but it gets us most of the way.
💬 willcl-ark commented on pull request "init: deduplicate added connections":
(https://github.com/bitcoin/bitcoin/pull/27804#issuecomment-1573700827)
<details>
<summary>Before and after connection summaries</summary>
```sh
# Node 1
/home/will/src/bitcoin/src/bitcoin-cli -datadir=/tmp/node1-datadir getpeerinfo | jq '[.[] | {id: .id, addr: .addr, type: .connection_type}]'
[
{
"id": 0,
"addr": "127.0.0.1:60216",
"type": "inbound"
},
{
"id": 1,
"addr": "127.0.0.1:60218",
"type": "inbound"
},
{
"id": 2,
"addr": "127.0.0.1:18400",
"type": "manual"
}
]
# Node 2
/home/will/s
...
(https://github.com/bitcoin/bitcoin/pull/27804#issuecomment-1573700827)
<details>
<summary>Before and after connection summaries</summary>
```sh
# Node 1
/home/will/src/bitcoin/src/bitcoin-cli -datadir=/tmp/node1-datadir getpeerinfo | jq '[.[] | {id: .id, addr: .addr, type: .connection_type}]'
[
{
"id": 0,
"addr": "127.0.0.1:60216",
"type": "inbound"
},
{
"id": 1,
"addr": "127.0.0.1:60218",
"type": "inbound"
},
{
"id": 2,
"addr": "127.0.0.1:18400",
"type": "manual"
}
]
# Node 2
/home/will/s
...
💬 ismaelsadeeq commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1214368208)
Estimates that are 2.5 days old have nearly half the decay weight of the most recent block
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1214368208)
Estimates that are 2.5 days old have nearly half the decay weight of the most recent block
🚀 glozow merged a pull request: "test: added coverage to mining_basic.py"
(https://github.com/bitcoin/bitcoin/pull/27603)
(https://github.com/bitcoin/bitcoin/pull/27603)
💬 glozow commented on issue "Package Relay Project Tracking":
(https://github.com/bitcoin/bitcoin/issues/27463#issuecomment-1573736784)
> If the state-of-art implementation of BIP331 can be linked here or in the BIP (Like there is https://github.com/glozow/bitcoin/pull/8 and https://github.com/glozow/bitcoin/pull/9) ?
> From a reviewer perspective, ideally it’s better to have the proposed specification changes, the proposed Core code changes and the backend code of broadcaster client (e.g Lightning) to grasp a correct mental model of all the interactions.
Forgot to respond to this earlier - #27742 is the branch you're lookin
...
(https://github.com/bitcoin/bitcoin/issues/27463#issuecomment-1573736784)
> If the state-of-art implementation of BIP331 can be linked here or in the BIP (Like there is https://github.com/glozow/bitcoin/pull/8 and https://github.com/glozow/bitcoin/pull/9) ?
> From a reviewer perspective, ideally it’s better to have the proposed specification changes, the proposed Core code changes and the backend code of broadcaster client (e.g Lightning) to grasp a correct mental model of all the interactions.
Forgot to respond to this earlier - #27742 is the branch you're lookin
...
💬 instagibbs commented on issue "-fallbackfee should apply to estimatesmartfee":
(https://github.com/bitcoin/bitcoin/issues/27415#issuecomment-1573751777)
Any long-running node has them if the node has ever restarted. I just grabbed one locally when needed.
(https://github.com/bitcoin/bitcoin/issues/27415#issuecomment-1573751777)
Any long-running node has them if the node has ever restarted. I just grabbed one locally when needed.
💬 instagibbs commented on issue "-fallbackfee should apply to estimatesmartfee":
(https://github.com/bitcoin/bitcoin/issues/27415#issuecomment-1573767151)
https://github.com/bitcoin/bitcoin/pull/27622 might want to keep an eye on this though, I think we should probably have a flag allowing any age of file, exposed on regtest/hidden only?
(https://github.com/bitcoin/bitcoin/issues/27415#issuecomment-1573767151)
https://github.com/bitcoin/bitcoin/pull/27622 might want to keep an eye on this though, I think we should probably have a flag allowing any age of file, exposed on regtest/hidden only?
👍 furszy approved a pull request: "walletdb: Add PrefixCursor"
(https://github.com/bitcoin/bitcoin/pull/27790#pullrequestreview-1457533319)
ACK ba616b93
Nice `db_cursor_prefix_byte_test` test case 👌🏼.
(https://github.com/bitcoin/bitcoin/pull/27790#pullrequestreview-1457533319)
ACK ba616b93
Nice `db_cursor_prefix_byte_test` test case 👌🏼.
💬 TheCharlatan commented on pull request "kernel: Remove shutdown from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27711#discussion_r1214417257)
Thank you for your comprehensive answer. I think it boils down to: A fatal error should not necessarily have to trigger an interrupt for all kernel operations. The user of the library should decide this. A `startShutdown` should interrupt all operations in order to cleanly halt kernel execution.
> I don't think it should be necessary for the kernel to call out to the application cancel it's own execution if it can just call the cancellation object directly.
Going back to this earlier rema
...
(https://github.com/bitcoin/bitcoin/pull/27711#discussion_r1214417257)
Thank you for your comprehensive answer. I think it boils down to: A fatal error should not necessarily have to trigger an interrupt for all kernel operations. The user of the library should decide this. A `startShutdown` should interrupt all operations in order to cleanly halt kernel execution.
> I don't think it should be necessary for the kernel to call out to the application cancel it's own execution if it can just call the cancellation object directly.
Going back to this earlier rema
...
🤔 instagibbs reviewed a pull request: "Fee estimation: avoid serving stale fee estimate"
(https://github.com/bitcoin/bitcoin/pull/27622#pullrequestreview-1457521354)
Looking good, just a couple of test suggestions.
Also wondering if we should include a hidden argument to allow older files for testing reasons, re: https://github.com/bitcoin/bitcoin/issues/27415
I've used it for systems I've built for integration testing, and not having to touch a file during testing to refresh it would be a simplification.
(https://github.com/bitcoin/bitcoin/pull/27622#pullrequestreview-1457521354)
Looking good, just a couple of test suggestions.
Also wondering if we should include a hidden argument to allow older files for testing reasons, re: https://github.com/bitcoin/bitcoin/issues/27415
I've used it for systems I've built for integration testing, and not having to touch a file during testing to refresh it would be a simplification.
💬 instagibbs commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1214407758)
to make this test less order-dependent, you can attempt to delete the file, then run the test
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1214407758)
to make this test less order-dependent, you can attempt to delete the file, then run the test
💬 instagibbs commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1214415612)
might be good to test that it does it *every hour*, and that the output files should be matching provided we don't see blocks in the interim, also, check that what is written on node shutdown also matches?
That way we can have higher assurance on the timing of the flushing and the contents matching what we already do.
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1214415612)
might be good to test that it does it *every hour*, and that the output files should be matching provided we don't see blocks in the interim, also, check that what is written on node shutdown also matches?
That way we can have higher assurance on the timing of the flushing and the contents matching what we already do.