⚠️ ZenulAbidin opened an issue: "An option for a shell command that runs just before bitcoind completes shutting down."
(https://github.com/bitcoin/bitcoin/issues/27984)
### Please describe the feature you'd like to see added.
It basically works similar to the blocknotify config option which runs a shell script when a block is mined, but the purpose of this new option is different.
It can be called something like "shutdowncomplete" and a shell command is passed to it as an argument, which then runs just before the following message is printed in debug.log:
```
2023-06-26T14:00:07Z Shutdown: done
```
It can be ran utilizing a function such as `runComm
...
(https://github.com/bitcoin/bitcoin/issues/27984)
### Please describe the feature you'd like to see added.
It basically works similar to the blocknotify config option which runs a shell script when a block is mined, but the purpose of this new option is different.
It can be called something like "shutdowncomplete" and a shell command is passed to it as an argument, which then runs just before the following message is printed in debug.log:
```
2023-06-26T14:00:07Z Shutdown: done
```
It can be ran utilizing a function such as `runComm
...
💬 fanquake commented on issue "An option for a shell command that runs just before bitcoind completes shutting down.":
(https://github.com/bitcoin/bitcoin/issues/27984#issuecomment-1609340699)
We already have `shutdownnotify`.
(https://github.com/bitcoin/bitcoin/issues/27984#issuecomment-1609340699)
We already have `shutdownnotify`.
🚀 fanquake merged a pull request: "Added static_assert to check that base_blob is using whole bytes."
(https://github.com/bitcoin/bitcoin/pull/27929)
(https://github.com/bitcoin/bitcoin/pull/27929)
💬 MarcoFalke commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#issuecomment-1609403231)
Also, it may be good to add tests for currently untested code: https://marcofalke.github.io/b-c-cov/total.coverage/src/wallet/wallet.cpp.gcov.html#2926
(https://github.com/bitcoin/bitcoin/pull/24914#issuecomment-1609403231)
Also, it may be good to add tests for currently untested code: https://marcofalke.github.io/b-c-cov/total.coverage/src/wallet/wallet.cpp.gcov.html#2926
💬 craigraw commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1609436322)
Using `getrawmempool true` is unfortunately not a solution that can be applied generally to solve this use case. On systems with low memory (4Gb or less, for example a Raspberry Pi or the Futurebit Apollo) this RPC call causes the kernel to terminate the Bitcoin Core process (OOM) when the mempool is reasonably full. This occurs consistently on all released versions AFAIK.
One workaround is to use `getrawmempool false` followed by repeated calls to `getmempoolentry`. This of course is even mo
...
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1609436322)
Using `getrawmempool true` is unfortunately not a solution that can be applied generally to solve this use case. On systems with low memory (4Gb or less, for example a Raspberry Pi or the Futurebit Apollo) this RPC call causes the kernel to terminate the Bitcoin Core process (OOM) when the mempool is reasonably full. This occurs consistently on all released versions AFAIK.
One workaround is to use `getrawmempool false` followed by repeated calls to `getmempoolentry`. This of course is even mo
...
💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1243705025)
It doesn't seem necessary to me, because the block arrival information is something that seems unrelated to setting up a chainstate (and I don't see why it would need to be reset in the event of a chainstate activation).
I think if we need to reset the counters it would be for tests, which is the only place outside of `init.cpp` where we use this... I thought I addressed adding `ResetBlockSequenceCounters()` where it was necessary, but I should look at that again to see if I missed a case.
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1243705025)
It doesn't seem necessary to me, because the block arrival information is something that seems unrelated to setting up a chainstate (and I don't see why it would need to be reset in the event of a chainstate activation).
I think if we need to reset the counters it would be for tests, which is the only place outside of `init.cpp` where we use this... I thought I addressed adding `ResetBlockSequenceCounters()` where it was necessary, but I should look at that again to see if I missed a case.
💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1243732248)
Good idea -- and it looks like my code here is wrong anyway, because if we're a disabled background chainstate, then we'll be adding blocks to setBlockIndexCandidates as long as they have more work than the tip!
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1243732248)
Good idea -- and it looks like my code here is wrong anyway, because if we're a disabled background chainstate, then we'll be adding blocks to setBlockIndexCandidates as long as they have more work than the tip!
💬 willcl-ark commented on pull request "build: debug enable addrman consistency checks":
(https://github.com/bitcoin/bitcoin/pull/27300#issuecomment-1609505161)
Nope, I also agree with the conclusions from AJ in 24709 about just setting this as a runtime option, so will close this.
(https://github.com/bitcoin/bitcoin/pull/27300#issuecomment-1609505161)
Nope, I also agree with the conclusions from AJ in 24709 about just setting this as a runtime option, so will close this.
✅ willcl-ark closed a pull request: "build: debug enable addrman consistency checks"
(https://github.com/bitcoin/bitcoin/pull/27300)
(https://github.com/bitcoin/bitcoin/pull/27300)
💬 sipa commented on pull request "Introduce secp256k1 module with field and group classes to test framework":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1243753860)
Done.
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1243753860)
Done.
💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1243773140)
Yeah my thought was that this notification wasn't something that made sense for the background chain -- the existing UI notifications for should be oblivious to background validation (and if we want to change that then we likely need new functionality/a new interface for whatever notifications we want to set up).
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1243773140)
Yeah my thought was that this notification wasn't something that made sense for the background chain -- the existing UI notifications for should be oblivious to background validation (and if we want to change that then we likely need new functionality/a new interface for whatever notifications we want to set up).
💬 sipa commented on pull request "test: add python implementation of Elligator swift":
(https://github.com/bitcoin/bitcoin/pull/24005#discussion_r1243781439)
Given that it's reduced to an `FE` anyway, arguably the correct value is `FE(random.randrange(1, FE.SIZE))`.
To be clear, there is no observable difference between using `GE.ORDER`, `2**256`, or `FE.SIZE`; they're all sufficiently close together that it doesn't matter, and the ElligatorSwift code in libsecp256k1 also relies on the fact that they're so close together.
But the "we prefer uniform u32 over uniform u" doesn't really apply here, as u is a field element, not a 32-byte array. In o
...
(https://github.com/bitcoin/bitcoin/pull/24005#discussion_r1243781439)
Given that it's reduced to an `FE` anyway, arguably the correct value is `FE(random.randrange(1, FE.SIZE))`.
To be clear, there is no observable difference between using `GE.ORDER`, `2**256`, or `FE.SIZE`; they're all sufficiently close together that it doesn't matter, and the ElligatorSwift code in libsecp256k1 also relies on the fact that they're so close together.
But the "we prefer uniform u32 over uniform u" doesn't really apply here, as u is a field element, not a 32-byte array. In o
...
💬 tobtoht commented on pull request "build: LLVM 15 & LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1609553593)
`powerpc64-linux-gnu` build is failing with:
```
Backtrace:
In srfi/srfi-1.scm:
586:29 19 (map1 (#<<manifest-entry> name: "python-lief" versio?> ?))
586:17 18 (map1 (#<<manifest-entry> name: "powerpc64-linux-gnu-t?>))
In guix/profiles.scm:
1930:19 17 (_ _)
In guix/packages.scm:
1348:17 16 (supported-package? #<package powerpc64-linux-gnu-tool?> ?)
In guix/memoization.scm:
101:0 15 (_ #<hash-table 7f27a126c4c0 282/443> #<package powerp?> ?)
In guix/packages.scm:
131
...
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1609553593)
`powerpc64-linux-gnu` build is failing with:
```
Backtrace:
In srfi/srfi-1.scm:
586:29 19 (map1 (#<<manifest-entry> name: "python-lief" versio?> ?))
586:17 18 (map1 (#<<manifest-entry> name: "powerpc64-linux-gnu-t?>))
In guix/profiles.scm:
1930:19 17 (_ _)
In guix/packages.scm:
1348:17 16 (supported-package? #<package powerpc64-linux-gnu-tool?> ?)
In guix/memoization.scm:
101:0 15 (_ #<hash-table 7f27a126c4c0 282/443> #<package powerp?> ?)
In guix/packages.scm:
131
...
💬 fanquake commented on pull request "build: LLVM 15 & LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1609558576)
> powerpc64-linux-gnu build is failing with:
Thanks, thats's already mentioned as a known issue here: #27897, which also does a time-machine bump. You don't need to build for powerpc to test the two macOS build, if you want to test the changes here.
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-1609558576)
> powerpc64-linux-gnu build is failing with:
Thanks, thats's already mentioned as a known issue here: #27897, which also does a time-machine bump. You don't need to build for powerpc to test the two macOS build, if you want to test the changes here.
💬 FelixWeis commented on issue "Assertion failed: (data.size() > node.nSendOffset), function SocketSendData, file net.cpp, line 837":
(https://github.com/bitcoin/bitcoin/issues/27963#issuecomment-1609560364)
```
2023-06-27T11:10:56.851484Z [mempool] AcceptToMemoryPool: peer=8: accepted 57303c3ef5097e684419aa326d2a0048005f1571c1b26ffa315b9b8119143907 (poolsz 40693 txn, 132600 kB)
2023-06-27T11:10:57.059933Z [net] sending inv (37 bytes) peer=2
2023-06-27T11:10:57.060104Z [net] socket sent 28 bytes (total: 24, offset: 28) for peer=2.
Assertion failed: (false), function SocketSendData, file net.cpp, line 859.
(https://github.com/bitcoin/bitcoin/issues/27963#issuecomment-1609560364)
```
2023-06-27T11:10:56.851484Z [mempool] AcceptToMemoryPool: peer=8: accepted 57303c3ef5097e684419aa326d2a0048005f1571c1b26ffa315b9b8119143907 (poolsz 40693 txn, 132600 kB)
2023-06-27T11:10:57.059933Z [net] sending inv (37 bytes) peer=2
2023-06-27T11:10:57.060104Z [net] socket sent 28 bytes (total: 24, offset: 28) for peer=2.
Assertion failed: (false), function SocketSendData, file net.cpp, line 859.
💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#issuecomment-1609563472)
With 3 up-to-date acks, and all the review that's happened so far, I think this looks ready to merge now. But will wait to see if @achow101 wants to respond to newest comments.
I like Marco's error handling suggestions since they simplify code and just print all the errors. If it turns out there are this cases where this repeats too many errors, it seems like it would be easy to add a bit of code to LoadRecords that just compares new error messages to previous and adds `[Repeated ## times]` t
...
(https://github.com/bitcoin/bitcoin/pull/24914#issuecomment-1609563472)
With 3 up-to-date acks, and all the review that's happened so far, I think this looks ready to merge now. But will wait to see if @achow101 wants to respond to newest comments.
I like Marco's error handling suggestions since they simplify code and just print all the errors. If it turns out there are this cases where this repeats too many errors, it seems like it would be easy to add a bit of code to LoadRecords that just compares new error messages to previous and adds `[Repeated ## times]` t
...
💬 MarcoFalke commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#issuecomment-1609571471)
> If it turns out there are this cases where this repeats too many errors
I think that'd be the case on current master as well. My comment was that this pull may repeat the *first* error message, even though there may be a second one.
(https://github.com/bitcoin/bitcoin/pull/24914#issuecomment-1609571471)
> If it turns out there are this cases where this repeats too many errors
I think that'd be the case on current master as well. My comment was that this pull may repeat the *first* error message, even though there may be a second one.
💬 MarcoFalke commented on issue "Assertion failed: (data.size() > node.nSendOffset), function SocketSendData, file net.cpp, line 837":
(https://github.com/bitcoin/bitcoin/issues/27963#issuecomment-1609577443)
So that seems to confirm my guess that this is a macOS bug? (The total data is 24 bytes, but macOS `send()` returned 28)
My suggestion would be to call Tim Cook or switch to Linux.
(https://github.com/bitcoin/bitcoin/issues/27963#issuecomment-1609577443)
So that seems to confirm my guess that this is a macOS bug? (The total data is 24 bytes, but macOS `send()` returned 28)
My suggestion would be to call Tim Cook or switch to Linux.
💬 ryanofsky commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#issuecomment-1609592218)
> I think that'd be the case on current master as well. My comment was that this pull may repeat the _first_ error message, even though there may be a second one.
Makes sense, I missed that. I thought the code was only trying to print one error message, and didn't notice it would print it multiple times. I like your fix of just printing all the messages instead of printing one, but either way to fix this seems fine.
(https://github.com/bitcoin/bitcoin/pull/24914#issuecomment-1609592218)
> I think that'd be the case on current master as well. My comment was that this pull may repeat the _first_ error message, even though there may be a second one.
Makes sense, I missed that. I thought the code was only trying to print one error message, and didn't notice it would print it multiple times. I like your fix of just printing all the messages instead of printing one, but either way to fix this seems fine.
💬 darosior commented on pull request "Wallet: estimate the size of signed inputs using descriptors":
(https://github.com/bitcoin/bitcoin/pull/26567#discussion_r1243851964)
Oops. Done now.
(https://github.com/bitcoin/bitcoin/pull/26567#discussion_r1243851964)
Oops. Done now.