💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1886844363)
> Want to punt the libtool removal to a follow-up PR to avoid slowing this one down?
Looks like the cmake changes will require some additional CI changes at least. I think we can still open an additional PR, with upnpc and natpmp, and get that merged. Seems cleanest to just nuke libtool here, and avoid interim workarounds for Ubuntus binarys etc. I'll pull your new commits in here in any case.
> If picking the clang version via depends is not a use case to support outside the CI,
We do
...
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1886844363)
> Want to punt the libtool removal to a follow-up PR to avoid slowing this one down?
Looks like the cmake changes will require some additional CI changes at least. I think we can still open an additional PR, with upnpc and natpmp, and get that merged. Seems cleanest to just nuke libtool here, and avoid interim workarounds for Ubuntus binarys etc. I'll pull your new commits in here in any case.
> If picking the clang version via depends is not a use case to support outside the CI,
We do
...
💬 moonsettler commented on pull request "Implement 64 bit arithmetic op codes in the Script interpreter":
(https://github.com/bitcoin/bitcoin/pull/29221#issuecomment-1886873235)
Concept ACK. i see no reason for this not to be bundled with any future amount introspection soft fork.
(https://github.com/bitcoin/bitcoin/pull/29221#issuecomment-1886873235)
Concept ACK. i see no reason for this not to be bundled with any future amount introspection soft fork.
📝 fanquake opened a pull request: "ci: move CMake into base packages"
(https://github.com/bitcoin/bitcoin/pull/29225)
This is already used in multiple CIs, and will soon become a requirement for most CIs, i.e when we migrate depends packages to use CMake, for example: https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1885576324.
Some of the CIs in 21778 are failing because CMake isn't available, so just break this out and make CMake globally available.
(https://github.com/bitcoin/bitcoin/pull/29225)
This is already used in multiple CIs, and will soon become a requirement for most CIs, i.e when we migrate depends packages to use CMake, for example: https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1885576324.
Some of the CIs in 21778 are failing because CMake isn't available, so just break this out and make CMake globally available.
💬 maflcko commented on pull request "ci: move CMake into base packages":
(https://github.com/bitcoin/bitcoin/pull/29225#issuecomment-1886940654)
Forgot centos?
(https://github.com/bitcoin/bitcoin/pull/29225#issuecomment-1886940654)
Forgot centos?
💬 fanquake commented on pull request "ci: move CMake into base packages":
(https://github.com/bitcoin/bitcoin/pull/29225#issuecomment-1886944344)
Forgot that has it's own `CI_BASE_PACKAGES`. Fixed up.
(https://github.com/bitcoin/bitcoin/pull/29225#issuecomment-1886944344)
Forgot that has it's own `CI_BASE_PACKAGES`. Fixed up.
📝 dergoegge opened a pull request: "Revert "build: Fix regression in "ARMv8 CRC32 intrinsics" test""
(https://github.com/bitcoin/bitcoin/pull/29226)
This reverts commit 228d6a2969e4fcee573c9df7aad31550eab9c8d4.
Fixes #29178
(https://github.com/bitcoin/bitcoin/pull/29226)
This reverts commit 228d6a2969e4fcee573c9df7aad31550eab9c8d4.
Fixes #29178
💬 dergoegge commented on pull request "Revert "build: Fix regression in "ARMv8 CRC32 intrinsics" test"":
(https://github.com/bitcoin/bitcoin/pull/29226#issuecomment-1886953885)
We could alternatively add a suppression but I prefer reverting unless someone can explain how this is not an actual bug.
(https://github.com/bitcoin/bitcoin/pull/29226#issuecomment-1886953885)
We could alternatively add a suppression but I prefer reverting unless someone can explain how this is not an actual bug.
💬 stickies-v commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1448721515)
I think ajtowns addressed that quite well at the bottom of [this comment](https://github.com/bitcoin/bitcoin/pull/28318#issuecomment-1702318968):
> I don't think that's a sensible concern: unconditional logging should be rare enough to not need disabling (eg, connecting to new outbounds, new headers, and new tips for about 400 entries a day), and at that point it's better not to disable them as that gives the user a pretty clear indication their node is still working right. Even for users th
...
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1448721515)
I think ajtowns addressed that quite well at the bottom of [this comment](https://github.com/bitcoin/bitcoin/pull/28318#issuecomment-1702318968):
> I don't think that's a sensible concern: unconditional logging should be rare enough to not need disabling (eg, connecting to new outbounds, new headers, and new tips for about 400 entries a day), and at that point it's better not to disable them as that gives the user a pretty clear indication their node is still working right. Even for users th
...
📝 glozow opened a pull request: "log mempool loading progress"
(https://github.com/bitcoin/bitcoin/pull/29227)
Motivated by #29193. Currently, we only log something (non-debug) when we fail to load the file and at the end of importing all the transactions. That means it's hard to tell what's happening if it's taking a long time to load.
This PR adds a maximum of 10 new unconditional log lines:
- When we start to load transactions.
- Our progress percentage when it advances by at least 10% from the last time we logged. Percentage is based on the number of transactions.
If there are lots of transac
...
(https://github.com/bitcoin/bitcoin/pull/29227)
Motivated by #29193. Currently, we only log something (non-debug) when we fail to load the file and at the end of importing all the transactions. That means it's hard to tell what's happening if it's taking a long time to load.
This PR adds a maximum of 10 new unconditional log lines:
- When we start to load transactions.
- Our progress percentage when it advances by at least 10% from the last time we logged. Percentage is based on the number of transactions.
If there are lots of transac
...
🚀 fanquake merged a pull request: "fuzz: Improve fuzzing stability for ellswift_roundtrip harness"
(https://github.com/bitcoin/bitcoin/pull/29219)
(https://github.com/bitcoin/bitcoin/pull/29219)
💬 maflcko commented on pull request "ci: move CMake into base packages":
(https://github.com/bitcoin/bitcoin/pull/29225#issuecomment-1886999559)
lgtm ACK f3ca6db8d349941f6bad6e8b328033222af8f063
(https://github.com/bitcoin/bitcoin/pull/29225#issuecomment-1886999559)
lgtm ACK f3ca6db8d349941f6bad6e8b328033222af8f063
💬 dergoegge commented on pull request "Revert "build: Fix regression in "ARMv8 CRC32 intrinsics" test"":
(https://github.com/bitcoin/bitcoin/pull/29226#issuecomment-1887003190)
cc @hebasto @maflcko
(https://github.com/bitcoin/bitcoin/pull/29226#issuecomment-1887003190)
cc @hebasto @maflcko
💬 dev7ba commented on issue "Failed loading mempool when restarting bitcoind":
(https://github.com/bitcoin/bitcoin/issues/29193#issuecomment-1887073369)
In retrospect, I believe what happened is that upon noticing the mempool wasn't loading, I gracefully shut down the node, causing mempool.dat to be overwritten with the limited contents of the mempool.
It might also be advisable not to overwrite the old mempool.dat if it hasn't finished loading completely.
(https://github.com/bitcoin/bitcoin/issues/29193#issuecomment-1887073369)
In retrospect, I believe what happened is that upon noticing the mempool wasn't loading, I gracefully shut down the node, causing mempool.dat to be overwritten with the limited contents of the mempool.
It might also be advisable not to overwrite the old mempool.dat if it hasn't finished loading completely.
💬 deyw-bit commented on issue "Failed loading mempool when restarting bitcoind":
(https://github.com/bitcoin/bitcoin/issues/29193#issuecomment-1887125546)
Upon examining the entire debug.log, which includes log entries from the past few months - it occasionally took up to nine hours to fully load the mempool on my node.
It's plausible that the mempool.dat could potentially be overwritten by the limited contents of the mempool if a shutdown is initiated before the loading of the mempool is completed.
`grep -E 'Shutdown|mempool|Bitcoin Core version v' /volume1/docker/bitcoin/debug.log`
2023-10-23T11:24:47Z Bitcoin Core version v25.0.0 (rel
...
(https://github.com/bitcoin/bitcoin/issues/29193#issuecomment-1887125546)
Upon examining the entire debug.log, which includes log entries from the past few months - it occasionally took up to nine hours to fully load the mempool on my node.
It's plausible that the mempool.dat could potentially be overwritten by the limited contents of the mempool if a shutdown is initiated before the loading of the mempool is completed.
`grep -E 'Shutdown|mempool|Bitcoin Core version v' /volume1/docker/bitcoin/debug.log`
2023-10-23T11:24:47Z Bitcoin Core version v25.0.0 (rel
...
💬 glozow commented on issue "Failed loading mempool when restarting bitcoind":
(https://github.com/bitcoin/bitcoin/issues/29193#issuecomment-1887152637)
@deyw-bit can you tell us the specs of your machine?
(https://github.com/bitcoin/bitcoin/issues/29193#issuecomment-1887152637)
@deyw-bit can you tell us the specs of your machine?
💬 maflcko commented on issue "Failed loading mempool when restarting bitcoind":
(https://github.com/bitcoin/bitcoin/issues/29193#issuecomment-1887158724)
> I gracefully shut down the node, causing mempool.dat to be overwritten with the limited contents of the mempool.
There is a check to retain the old mempool in that case. So if you never get along to writing the new mempool, due to repeated restarts, the new transactions will be discarded.
(https://github.com/bitcoin/bitcoin/issues/29193#issuecomment-1887158724)
> I gracefully shut down the node, causing mempool.dat to be overwritten with the limited contents of the mempool.
There is a check to retain the old mempool in that case. So if you never get along to writing the new mempool, due to repeated restarts, the new transactions will be discarded.
💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1887170261)
> One can always use a revoked state with max outgoing HTLC outputs=483 to pin.
> So 483 * 33 + 1000, you have ~18k vbytes state, mempool backlog at 20 sat/vb, you can steal ~100 USD payments.
Ok @instagibbs helped me parse what you're trying to say here (thanks) and IIUC you're talking about a scenario in which:
- The counterparties in a channel have negotiated a very high number of maximum in-flight HTLCS, so they have at some point signed a commitment transaction that was 18KvB in size.
...
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1887170261)
> One can always use a revoked state with max outgoing HTLC outputs=483 to pin.
> So 483 * 33 + 1000, you have ~18k vbytes state, mempool backlog at 20 sat/vb, you can steal ~100 USD payments.
Ok @instagibbs helped me parse what you're trying to say here (thanks) and IIUC you're talking about a scenario in which:
- The counterparties in a channel have negotiated a very high number of maximum in-flight HTLCS, so they have at some point signed a commitment transaction that was 18KvB in size.
...
📝 maflcko opened a pull request: "test: Remove all-lint.py script"
(https://github.com/bitcoin/bitcoin/pull/29228)
Seems confusing to have a test runner that calls another runner (`all-lint.py`), which calls a subset of the lint tests.
Fix that by just calling this subset of lint tests in the test runner directly, and remove the now unused `all-lint.py`.
(https://github.com/bitcoin/bitcoin/pull/29228)
Seems confusing to have a test runner that calls another runner (`all-lint.py`), which calls a subset of the lint tests.
Fix that by just calling this subset of lint tests in the test runner directly, and remove the now unused `all-lint.py`.
💬 dergoegge commented on pull request "log mempool loading progress":
(https://github.com/bitcoin/bitcoin/pull/29227#issuecomment-1887174559)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/29227#issuecomment-1887174559)
Concept ACK
🤔 furszy reviewed a pull request: "wallet: Refactor DumpWallet function to accept -dumpfile path argument"
(https://github.com/bitcoin/bitcoin/pull/29223#pullrequestreview-1815601947)
The rationale behind this was to remove the `<common/args.h>` include from dump.cpp and the `ArgsManager` type from `dump.h` (which are missing here). Also, should pass `fs::path` instead of a string.
This cleans up code but.. it was a tiny nit that I would group with other changes. Making tiny refactoring PRs like this isn't really the best (we could have dozens of it meanwhile we only have a handful amount of reviewers).
(https://github.com/bitcoin/bitcoin/pull/29223#pullrequestreview-1815601947)
The rationale behind this was to remove the `<common/args.h>` include from dump.cpp and the `ArgsManager` type from `dump.h` (which are missing here). Also, should pass `fs::path` instead of a string.
This cleans up code but.. it was a tiny nit that I would group with other changes. Making tiny refactoring PRs like this isn't really the best (we could have dozens of it meanwhile we only have a handful amount of reviewers).